fix: preserve split-form (--flag value) user-managed broker-router co…

[aniketpandey05]

What does this PR do?

Fixes the broker-router command reconciliation logic to correctly handle user-managedflags written in split form (--flag value) instead of only --flag=value.Previously mergeCommand dropped the value token of such flags on reconcile, and
filterManagedFlags could report false drift for the orphaned value token.

Details

internal/controller/broker_router.go:

• Added commandEntry / parseCommandEntries to split a container command into logical entries (binary name, --flag=value, --flag value pair, or bare --flag).
• Rewrote filterManagedFlags and mergeCommand to operate on these logical entries instead of raw argv tokens, so split-form flags and their valuesare preserved/compared as a unit.
• Switched managed-flag matching from strings.HasPrefix to an exact matchon the flag name (extracted before =), avoiding accidental prefixcollisions. All existing managed flags are not prefixes of one another, soexisting behavior is unchanged.

internal/controller/deployment_test.go:

• TestFilterManagedFlags: added cases for a user split-form flag (stripped)and a managed split-form flag (kept as a unit).
• TestMergeCommand: added cases for preserving user split-form flags, mixedmanaged =-form + user split-form flags, and a managed split-form flagbeing replaced by the desired =-form value.
• TestDeploymentNeedsUpdate: added a no-op case for a user-managedsplit-form flag (previously reported false drift).

Summary by CodeRabbit

• Refactor

  • Improved command-parsing to treat command tokens as flag entries, correctly handling both --flag=value and split --flag value forms; preserves binary name and groups flag/value pairs.

• Tests

  • Added coverage ensuring user split-form flags are preserved/ignored as intended and managed flags are merged or replaced correctly across mixed scenarios.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4c5148b2-0447-4682-bdd3-8b69de8e3013

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[aniketpandey05]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[Patryk-Stefanski]

The underlying bugs here are real and worth fixing — the prefix-match collision and the dropped split-form value token are genuine. Thanks for tackling this.

That said, I think the tokenizer is more machinery than the problem requires. The controller's own buildBrokerRouterDeployment always emits --flag=value form (the only exception is the bare --enable-url-elicitation), so the split-form parser only ever activates on user-added flags in the existing deployment. The tokenizer also has an edge case: values that themselves start with -- (e.g. --my-flag --some-value) are rejected by the !strings.HasPrefix(command[i+1], "--") lookahead, silently splitting the pair into two separate entries.

The simpler approach that fixes both original bugs without the new one:

func extractFlagName(arg string) string {
    if i := strings.Index(arg, "="); i != -1 {
        return arg[:i]
    }
    return arg
}

Then replace the strings.HasPrefix(arg, flag) calls in filterManagedFlags and mergeCommand with slices.Contains(managedCommandFlags, extractFlagName(arg)). This fixes the prefix-collision bug exactly and is immune to the ---prefixed value edge case.

The trade-off: a user split-form --flag value pair would still result in only --flag being dropped and the orphaned value token being kept as a spurious user token — but that's a narrow edge case (split-form flags in a Kubernetes container command are uncommon) and the simpler code is easier to reason about.

Happy to discuss if there's a strong reason to handle that case too.

[aniketpandey05]

@Patryk-Stefanski I think your fix is better as it resolves both real bugs (the prefix-collision and the dropped split-form value in filterManagedFlags/mergeCommand), and it's easier to review

One question should we also update filterManagedFlags to compare by exact flag name/index instead of the current !strings.HasPrefix(arg, "--") check? That check currently lets any non-flag token through, including a stray value from a split-form flag, which could make the comparison see "drift" that isn't really there and trigger unnecessary updates on every reconcile.

thank you for revieiwing the pr

[Patryk-Stefanski]

One rename request before merge, and an answer to your question below.

**Rename function → ** (or similar)

The package-level function and the struct field share the same identifier. Go resolves them correctly by context, but a reader scanning the struct sees and the function named in the same file and has to stop to distinguish them. Something like or removes the ambiguity for no cost.

---

Your question about filterManagedFlags

Should we also update filterManagedFlags to compare by exact flag name/index instead of the current !strings.HasPrefix(arg, "--") check? That check currently lets any non-flag token through, including a stray value from a split-form flag, which could make the comparison see "drift" that isn't really there.

That's exactly the bug this PR fixes — and the fix is already here. The old token-at-a-time loop admitted stray value tokens (like "3600" from --session-length 3600) because they don't start with --, so !strings.HasPrefix(arg, "--") passed them through. The new parseCommandEntries groups the flag and its value into a single commandEntry, so a stray value can never appear as a standalone entry. The new filterManagedFlags then only needs to check entry.flagName == "" (binary/positional) and isManagedFlag(entry.flagName). No further change needed — the PR is the answer to your own question.

[Patryk-Stefanski]

The function name flagName collides with the struct field commandEntry.flagName — both live in the same file and share the identifier. Consider renaming to extractFlagName or parseFlagName to make them distinct at a glance.

[aniketpandey05]

@Patryk-Stefanski I have renamed the package-level helper function to extractFlagName to eliminate any ambiguity with the commandEntry.flagName struct field.

Thanks for the clarification on filterManagedFlags