Solid CLI surface. fda pipeline tag list|set|add|remove with --pipeline to scope, sensible aliases (ls, rm), good help text, JSON and text output paths, color-aware text formatting that turns itself off for non-TTY / NO_COLOR. The exhaustive unit-test coverage of the pure functions in tags.rs is the right shape — every invariant (variant exclusivity, lexicographic order, color borrowing, idempotence, rename-collapses-into-existing-target) has a named test. That makes the next person who touches this code much safer.

But there are real design choices and a few concrete issues that deserve attention before this lands:

Architectural: in-band color encoding via name|rrggbb

The whole color scheme is built on stuffing the color into the tag string itself, with the parser deciding "if the last |-segment is exactly six hex digits, it's a color." This is clever — it requires zero backend changes — but it's a form of in-band signaling, which has a couple of long-term costs:

• A legitimate name ending in six hex digits is misinterpreted. A user who tags a pipeline release|abcdef (a literal name they meant) loses the suffix in display, gets colored in the terminal, and discovers the tag isn't what they thought it was. The module doc and the tests acknowledge "uppercase hex digits are a valid color" but don't acknowledge that users will, in practice, hit this collision. Worth at least one error-case test that documents the behavior (e.g., a comment in display_name_strips_color_suffix saying "this is the unavoidable cost of in-band encoding; users naming tags with six hex digits after a | will see them rendered as colored").
• Backend doesn't validate. The backend accepts any string passing its character set; the color contract lives entirely in the CLI. A web UI (#6506) that uses a different convention — say, :rrggbb — would silently disagree. Worth a paragraph in tags.rs module doc saying "this convention must match #6506's encoding, see file X line Y" — and ideally extracting the encoding helpers to a shared crate (feldera-tag-format or similar) that both the CLI and the web console depend on. As written, drift between fda and the web console is one merge conflict away.

If you'd rather not extract a shared crate now, at least extract the encoding to a const SEPARATOR: char = '|' and a const COLOR_LEN: usize = 6 so the magic numbers have names.

Concurrency / consistency: TOCTOU on cross-pipeline operations

rename_tag_everywhere does:

1. get_pipelines_with_tags(client).await — list all pipelines + their tags.
2. Compute updates locally.
3. set_many_pipeline_tags(client, &updates).await — patches sent in batches of 20.

Between step 1 and step 3, another client (or another fda invocation) can:

• Create a new pipeline with the source tag → it gets missed by the rename.
• Patch a pipeline's tags → our patch_pipeline overwrites their change because we send the full tag array (tags: Some(tags)).

The second one is the worse risk because it's silent data loss for an unrelated tag change. The current patch_pipeline interface forces a full-list replacement, so the right fix is either:

• Optimistic concurrency. Check tag list at the time of patch (server-side conditional update keyed by an ETag / version), bail and tell the user "race detected, re-run" if it differs.
• Server-side tag delta API. A PATCH /pipeline/{name}/tags with add/remove semantics, so a client doesn't have to read-modify-write the whole tag set to add a single tag. This is what most tag systems converge on for exactly this reason.

This PR doesn't need to fix that today, but it does need to acknowledge it. Worth a paragraph in set_many_pipeline_tags doc saying "this read-modify-write loses concurrent tag changes on the same pipelines; we accept this for now because tag operations are infrequent" and an issue tracking the proper fix.

get_known_tags is called per-mutation and scans every pipeline

In TagAction::Add, TagAction::Set, and PipelineAction::Create, every mutation does a list_pipelines + flat_map. For a deployment with thousands of pipelines, every fda pipeline tag add --pipeline foo bar walks the entire pipeline list. That's fine for a few hundred pipelines, not great at scale.

• Worth a // TODO: cache get_known_tags within an fda invocation comment, or better, a server-side GET /tags endpoint that returns the distinct set of tag strings (and their counts). The web-console PR #6506 will hit the same N+1, so this is a natural shared endpoint.

set_many_pipeline_tags error handling drops failures silently

async fn set_many_pipeline_tags(client: &Client, updates: &[(String, Vec<String>)]) {
    for batch in updates.chunks(TAG_PATCH_BATCH_SIZE) {
        futures::future::join_all(
            batch.iter().map(|(name, tags)| set_pipeline_tags(client, name, tags.clone())),
        ).await;
    }
}

set_pipeline_tags uses handle_errors_fatal(...).unwrap(), which exit(1)s on the first failure. In a join_all batch, any error inside the batch will abort the process while peers continue in flight; the user gets no indication of which pipeline actually failed, and we may have partially applied the rename. Two issues:

1. The "partially applied rename" leaves the cluster in a state where, say, half the pipelines have prod renamed to production and half don't. There's no transactional boundary, and no way to detect or roll back. Worth at least one log line per success so an operator knows where the rename stopped.
2. handle_errors_fatal calling exit(1) from inside a future is a hard-to-debug pattern. Prefer returning Result and aggregating across the batch, then printing all failures at the end with a single non-zero exit.

This is also a place where the "Set" or "Rename" commands should print before mutating ("This will rename prod to production on 47 pipelines. Continue? [y/N]") for any cross-pipeline operation, perhaps gated on --yes. A typo'd rename across a production deployment is currently a one-line accident.

report_affected_pipelines is called with empty changed

I don't see report_affected_pipelines in the snippet I read but I'm inferring from the changed: Vec<String> plumbing in rename_tag_everywhere / remove_tag_everywhere. When no pipelines carry the source tag, both functions silently no-op. Worth printing "No pipelines carry <source>; nothing to rename." for the text path so a user with a typo'd source name doesn't think the rename succeeded.

Smaller observations

• tags.rs is 521 lines of pure logic with no I/O dependencies. This is great. Worth extracting to a feldera-tags crate so #6506 can depend on it instead of reimplementing the encoding (this is the shared-crate suggestion in concrete form).
• tag_color_rgb doc says "the parse cannot fail." Then why the ? on each from_str_radix(...).ok()?? You can unwrap() or restructure to return the tuple directly, since is_hex_color has already validated. Minor.
• variant_index sorts the whole pool to make the choice deterministic. Comment says "lexicographically smallest one wins." Fine, but sort_unstable is enough since you don't care about equal-key stability. Save a few allocations on big pools.
• add_tags filters additions by !own.contains_key(display_name) and then calls resolve_variant(tag, &own, &known) — at that point own.get(display_name) is guaranteed None, so the own lookup is dead. Either drop the filter (and let resolve_variant win) or drop the own argument in this code path. The current shape works but has a redundant lookup; the test add_preserves_existing_color_variant covers the case but doesn't pin the dead-lookup detail. Refactoring would be cleaner: one entry point with one rule.
• get_pipeline_tags does a full pipeline fetch (get_pipeline) just to read .tags. If there's a lighter endpoint or a projection (?fields=tags), use it. Same for list_pipelines — we pull every pipeline's full state to flatten to (name, tags). Server-side projection or a dedicated GET /tags would eliminate the cost.
• TAG_PATCH_BATCH_SIZE = 20. Good explicit bound, satisfies Power-of-Ten #2 (bounded resource use). Document the choice — why 20 specifically? Backend rate-limit boundary? Tested empirically? A one-line "20 keeps total in-flight requests below the connection pool default" or similar would help.
• Help text for Set without --pipeline describes "exactly two tags: source and target." It would be friendlier as an explicit Rename { source, target } subcommand. Conflating set-and-rename behind argument-count discrimination is the kind of overloading that breaks shell completion (the completer can't predict what the next positional means) and trips users. Consider a separate TagAction::Rename subcommand for the cross-pipeline case.

Verdict

The shape is right, the encoding is clever, and the test coverage of the pure logic is excellent. But the in-band color encoding needs to be a shared module with #6506, the cross-pipeline operations need at least a "renamed N pipelines" / "this will affect N pipelines, continue?" guardrail, and set_many_pipeline_tags needs proper error aggregation instead of exit(1) from inside a join_all. I'd want at minimum the error-handling and the destructive-action confirmation before this lands.

Marking as COMMENT to defer the verdict to gz (the requested reviewer) — the architectural call on in-band-encoding vs. server-side-tag-color belongs upstream.

Scope is much tighter than the previous revision — fda pipeline tag list|set|add|remove and the cross-pipeline rename_tag_everywhere / remove_tag_everywhere / set_many_pipeline_tags machinery are all gone. What lands is fda tags <name> (read), fda set-tags <name> <csv> (replace), and --tag on pipeline create. That sidesteps the three real concerns from my prior pass: the TOCTOU on multi-pipeline rename, the exit(1) inside a join_all batch, and the destructive-action-without-confirmation surface. Good call backing those out — they belong behind a server-side delta API, not a CLI loop.

crates/fda/src/tags.rs is still the same well-isolated, exhaustively-tested pure module it was — the encoding/normalization/variant-resolution invariants are pinned by named tests, and TAG_PATCH_BATCH_SIZE is no longer reachable so the bound becomes a no-op (fine to leave; harmless and documents intent if a multi-pipeline path returns later).

The in-band name|rrggbb color encoding still has the long-term drift risk against #6506 (web console), but that's now a much smaller blast radius — just the two read/write commands — and worth resolving when the shared crate lands rather than blocking here. Worth a follow-up issue: "extract tag color encoding to shared crate, used by fda and web-console" so the convention doesn't fork.

Approving. Looks good.