fix(cli): Fix admin cli parsing multiple config values

[neil-xie]

What changed?
Fixed the bug in the CLI's admin config update command, it can not parse multiple values with comma separated

Why?
The command cadence admin config update --name frontend.validSearchAttributes --value '{"Value":{"DomainID":1,"WorkflowID":1}}' was failing with the error "unexpected end of JSON input".

Root Cause: The urfave/cli v2 library's StringSliceFlag splits values on commas by default. When you passed JSON containing commas like {"Value":{"DomainID":1,"WorkflowID":1}}, it was being split into multiple pieces:
"{"Value":{"DomainID":1" and ""WorkflowID":1}}", which broke the JSON parsing.

How did you test it?
go test -v ./tools/cli -run TestAdminUpdateDynamicConfig

Potential risks
Only affects admin cli tool to update configs, no impact to file based configs

Release notes

Documentation Changes

---

Reviewer Validation

PR Description Quality (check these before reviewing code):

• "What changed" provides a clear 1-2 line summary
  • Project Issue is linked
• "Why" explains the full motivation with sufficient context
• Testing is documented:
  • Unit test commands are included (with exact go test invocation)
  • Integration test setup/commands included (if integration tests were run)
  • Canary testing details included (if canary was mentioned)
• Potential risks section is thoughtfully filled out (or legitimately N/A)
• Release notes included if this completes a user-facing feature
• Documentation needs are addressed (or noted if uncertain)

[gitar-bot]

🔍 CI failure analysis for 3e41e2a: The CI failure in TestWatchLoopDisabled is 100% reproducible across four consecutive runs and unrelated to the PR changes, which only modify CLI config parsing code.

Issue

Test failure in github.com/uber/cadence/service/sharddistributor/client/spectatorclient

Root Cause

The test TestWatchLoopDisabled consistently fails with the error "An error is expected but got nil" at line 285 of clientimpl_test.go. This test expects an error to be returned when the watch loop is disabled, but the implementation returns nil instead.

This failure has now occurred in four consecutive CI runs (job IDs: 64212973854, 64224949030, 64226978603, 64231234032), confirming it's a highly reproducible issue affecting the master branch, not a flaky test.

Details

The failure details:

• Package: github.com/uber/cadence/service/sharddistributor/client/spectatorclient
• Test: TestWatchLoopDisabled
• File: service/sharddistributor/client/spectatorclient/clientimpl_test.go:285
• Error: An error is expected but got nil
• Status: 100% reproducible across four consecutive runs

This test failure is completely unrelated to the PR changes. The PR only modifies:

• tools/cli/admin_config_store_commands.go (admin CLI config parsing)
• tools/cli/admin_config_store_commands_test.go (tests for admin CLI)

The failing test is in the service/sharddistributor/client/spectatorclient package, which is in a completely different part of the codebase. This is a pre-existing issue in the master branch that needs to be addressed separately.

Code Review ✅ Approved 1 resolved / 1 findings

Clean fix with the previous finding resolved — the workaround now always validates reassembled JSON before accepting it, and correctly falls back to the raw array for legitimate multiple-value inputs.

✅ 1 resolved

✅ Edge Case: Case 1 reassembles without JSON validation, unlike Case 2

📄 tools/cli/admin_config_store_commands.go:121
The first branch (line 121) blindly reassembles comma-split values without validating the result is valid JSON, while the second branch (line 124) correctly validates with json.Unmarshal before committing to the reassembled value.

Although it's difficult to construct a realistic scenario where this causes a problem (valid JSON objects always end with } so Case 1 rarely triggers for valid input), the inconsistency is unnecessary. If Case 1 were triggered by non-JSON input that happens to start with { but whose last fragment doesn't end with }, it would blindly join the fragments and pass garbage downstream. The downstream unmarshal at line 152 would catch it, but the error message ("Unable to unmarshal value to inputValue") would be confusing.

A simpler and more robust approach: unify Cases 1 and 2 into a single branch that always attempts reassembly and validates with json.Unmarshal, falling back to raw values on failure. This eliminates the special-case logic and makes the code easier to reason about.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}