watch: accept a comma-separated vault list

[bborbe]

What

vault-cli watch accepts a comma-separated vault list: --vault a,b,c runs one watcher process covering exactly those vaults, emitting one newline-delimited JSON event stream in which every event still carries its own vault field.

Why

A consumer that displays several vaults — vault-ui starts one watcher subprocess per configured vault — pays for a flag that cannot express a list: N processes, N descriptor sets, N restart loops, N event streams. The watching engine already accepted a []WatchTarget slice and already stamped each event with its vault. Only the command-line boundary was restricted to a single name.

Behaviour

• --vault a,b → one process, exactly those vaults.
• --vault a → unchanged. No --vault → unchanged (every configured vault).
• Whitespace around names ignored ("a, b"); empty entries ignored (a,,b).
• A value naming no vault (",", " ") errors rather than silently widening to every vault.
• An unresolvable name errors naming that vault; no partial watcher starts.
• --vault semantics are watch-scoped: getVaults and the root persistent flag registration are untouched, so every other command behaves exactly as before.

Verification

• make precommit green.
• A unit table over the list parser, plus integration specs that drive the real binary against a two-vault config and assert an event for each vault — a build that watches only the first name fails.
• task list --vault alpha,beta still fails with the pre-change vault not found error. That is the check separating this from the eight-line shortcut of splitting the value inside the shared resolver.
• The watching engine is pinned by content hash rather than a git diff: pkg/ops/watch.go sha256, its 198 lines, and 60 .go files under pkg/ops/. (The container runs with hideGit=true, so a git check would fail and report a false pass — the daemon does not check verification exit codes.)

Spec: specs/in-progress/047-watch-comma-separated-vault-list.md

[ben-s-pull-request-reviewer]

Chunk 1/4

The mock Returns() methods return no values (they're setters), so the no-bare-error-call findings are false positives from the funnel. The suite file pkg/cli/cli_suite_test.go exists, so the suite-test-file-required findings are also false positives. Now I'll produce the final report.

---

PR Review: watch: accept a comma-separated vault list

Changed Files (Chunk 1/4)

File	Status
CHANGELOG.md	✅ Well-written, describes behavior precisely
docs/development-patterns.md	✅ Updated to document new getWatchVaults function
pkg/cli/cli.go	✅ getWatchVaults function added; createWatchCommand updated to use it
pkg/cli/watch_test.go	✅ Comprehensive unit tests for all edge cases
pkg/cli/export_test.go	✅ Exposes GetWatchVaultsForTest for package-internal testing
integration/watch_test.go	✅ Full integration tests covering AC1–AC4 + DB6

Must Fix (Critical)

None.

Should Fix (Important)

None — the mechanical funnel findings are all false positives:

• pkg/cli/watch_test.go:116,131,146,147,164,165,179,180,195,242,243 — The ast-grep rule go-testing/no-bare-error-call fires on mock Returns() / ReturnsOnCall() calls because they have an error return signature in the interface. However, Loader.GetVaultReturns(*config.Vault, error) and GetAllVaultsReturns([]*config.Vault, error) are setter methods that return no values — they only record stub state for later retrieval. There is nothing to discard. The findings are wrong; Expect(fn()).To(Succeed()) wrapping would be equally incorrect.
• pkg/cli/cli.go:64,76 — go-context/cancel-check-in-loop (SHOULD). The parsing loop (line 64) is bounded by strings.Split, trivially short. The vault-resolution loop (line 76) is bounded by the number of names in the comma list (typically 1–3). Adding select { case <-ctx.Done(): ...; default: } to each iteration would be noise for such tightly bounded, non-blocking loops. The context is already passed to the underlying GetVault calls where cancellation is meaningful. No change needed.

Nice to Have (Optional)

None.

Notes

• Separation of concerns is correct: getWatchVaults handles the comma-list parsing, getVaults remains unchanged for all other commands. The spec requirement "no other command's --vault accepts a comma list" is enforced by keeping the functions separate.
• Edge cases all covered: empty segments (alpha,,beta), whitespace (alpha , beta), pure-separator values (,, ,,, ), and unresolvable names are all tested at both unit and integration levels.
• integration/watch_test.go is a new file and has no suite file concern — integration/integration_suite_test.go exists.

---

{
  "verdict": "approve",
  "summary": "Comma-separated vault list for `watch` command is well-implemented with correct edge-case handling, comprehensive test coverage at both unit and integration levels, and proper separation from the existing single-vault `getVaults` path. All mechanical funnel findings are false positives: mock setter methods return nothing to discard, suite files exist, and the loops are trivially bounded.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: comma-separated vault list parsing — verify empty segments are handled (e.g. \"vault1,,vault2\")",
      "disposition": "addressed",
      "detail": "getWatchVaults at cli.go:67 skips empty entries after TrimSpace; unit test 'skips empty entries between commas' and integration test AC3 cover this"
    },
    {
      "concern": "correctness: whitespace handling around commas — \"vault1, vault2\" vs \"vault1,vault2\"",
      "disposition": "addressed",
      "detail": "getWatchVaults at cli.go:66 calls strings.TrimSpace on each entry; unit test 'ignores whitespace around each name' and integration test AC1 cover this"
    },
    {
      "concern": "correctness: trailing/leading comma edge cases — \",vault1\" or \"vault1,\"",
      "disposition": "addressed",
      "detail": "TrimSpace on empty segments produces \"\", which is skipped by the continue; test 'fails when the value is only a comma' and 'fails when the value is only separators' cover this"
    },
    {
      "concern": "tests: unit tests should cover empty segments, whitespace, and leading/trailing commas",
      "disposition": "addressed",
      "detail": "All edge cases tested in pkg/cli/watch_test.go with dedicated It blocks"
    },
    {
      "concern": "tests: integration tests should exercise multi-vault watch scenarios",
      "disposition": "addressed",
      "detail": "integration/watch_test.go covers AC1 (both vaults), AC2 (single vault isolation), AC3 (failure modes), AC4 (other commands unaffected), DB6 (--types filter), and missing task directory"
    }
  ]
}

Chunk 2/4

I now have sufficient context. Let me adjudicate.

Classification:

• go-testing/no-bare-error-call (MUST): The 14 flagged calls are all to mock setter methods (SetErr, SetOut, SetArgs, GetAllVaultsReturns, GetVaultReturns, GetVaultReturnsOnCall) — these Counterfeiter-generated methods take values and return nothing. The rule cannot see this from the AST alone. All are false positives.
• go-testing/suite-test-file-required (MUST): The suite file pkg/cli/cli_suite_test.go exists and contains RunSpecs. All three flagged locations are false positives — the suite file is shared across all specs in the package.

Adjudication: No rule violations found. The test file has comprehensive coverage of comma-separated vault list parsing (empty segments, whitespace, leading/trailing commas, duplicate names, case-insensitive lookup) across 12 unit test cases.

{
  "verdict": "approve",
  "summary": "Tests for the comma-separated vault list feature are well-structured with comprehensive edge-case coverage. All mechanical funnel findings are false positives: mock setter methods have no return values (ast-grep cannot see this), and the suite file already exists.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: comma-separated vault list parsing — verify empty segments are handled",
      "disposition": "addressed",
      "detail": "pkg/cli/watch_test.go:179-193 — test 'skips empty entries between commas' covers \"alpha,,beta\""
    },
    {
      "concern": "correctness: whitespace handling around commas",
      "disposition": "addressed",
      "detail": "pkg/cli/watch_test.go:164-177 — test 'ignores whitespace around each name' covers \" alpha , beta \""
    },
    {
      "concern": "correctness: trailing/leading comma edge cases",
      "disposition": "addressed",
      "detail": "pkg/cli/watch_test.go:208-240 — tests cover \",\" (comma only), \" ,,\" (double comma), and \" \" (whitespace only)"
    },
    {
      "concern": "tests: unit tests should cover empty segments, whitespace, and leading/trailing commas",
      "disposition": "addressed",
      "detail": "12 unit tests in watch_test.go cover all specified edge cases"
    },
    {
      "concern": "tests: integration tests should exercise multi-vault watch scenarios",
      "disposition": "not-an-issue",
      "detail": "integration/watch_test.go is reviewed in chunk 1/4 — not in scope for this chunk"
    }
  ]
}

Chunk 3/4

I have sufficient information. The mechanical funnel found 0 findings (ast-grep not available in this pod), and the Step 4b-i candidate computation requires a rules index I cannot reliably query in this environment. Let me proceed directly to Step 7 — manual review against the Plan concerns.

Manual review of pkg/cli/cli.go — getWatchVaults (lines 48–85):

The concerns from the Plan map directly to the code:

Concern	Code	Verdict
Empty segments ("vault1,,vault2")	name := strings.TrimSpace(entry); if name == "" { continue } — empty after trim is skipped	✅ Correct
Whitespace handling ("vault1, vault2")	name := strings.TrimSpace(entry) — trims before the empty-check	✅ Correct
Trailing/leading comma (",vault1" / "vault1,")	Both produce "" after trim, skipped; if all entries skip → len(names) == 0 → error	✅ Correct
Error on empty-only value ("," / " ")	len(names) == 0 triggers errors.Errorf	✅ Correct
Single name and omitted flag	Early returns for *vaultName == "" and non-comma cases	✅ Correct

Unit tests in pkg/cli/watch_test.go cover all these cases (lines 116–285). Integration tests in integration/watch_test.go cover AC1–AC4 and DB6. The export_test.go correctly exposes getWatchVaults via GetWatchVaultsForTest.

Other observations:

• docs/development-patterns.md changed — the diff was excluded from the chunk scope.
• CHANGELOG.md entry correctly describes the feature without version number.
• Spec/prompt files are documentation, not code.
• No other command's --vault was changed (the getVaults function is untouched); the integration test at line 165 of integration/watch_test.go verifies this.

The implementation is correct and well-tested. There are no findings.

---

{
  "verdict": "approve",
  "summary": "The watch command now accepts a comma-separated vault list via a new getWatchVaults function that correctly handles empty segments, whitespace trimming, leading/trailing commas, and error cases for invalid input. Unit and integration tests cover all edge cases. No other command's --vault flag was affected.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: comma-separated vault list parsing — verify empty segments are handled (e.g. \"vault1,,vault2\")",
      "disposition": "addressed",
      "detail": "cli.go:66-68: empty entries are trimmed then skipped via continue; len(names)==0 check on line 72 returns error if nothing usable remains"
    },
    {
      "concern": "correctness: whitespace handling around commas — \"vault1, vault2\" vs \"vault1,vault2\"",
      "disposition": "addressed",
      "detail": "cli.go:66: strings.TrimSpace(entry) is called before the empty check, so whitespace on either side of a name is ignored"
    },
    {
      "concern": "correctness: trailing/leading comma edge cases — \",vault1\" or \"vault1,\"",
      "disposition": "addressed",
      "detail": "cli.go:66-68: both produce \"\" after TrimSpace and are skipped via continue; if all entries skip, len(names)==0 on line 72 triggers error"
    },
    {
      "concern": "tests: unit tests should cover empty segments, whitespace, and leading/trailing commas",
      "disposition": "addressed",
      "detail": "watch_test.go lines 164-240 cover all these cases: whitespace (line 164), empty entries (line 179), leading/trailing comma (line 208), only separators (line 231)"
    },
    {
      "concern": "tests: integration tests should exercise multi-vault watch scenarios",
      "disposition": "addressed",
      "detail": "integration/watch_test.go covers AC1 (multi-vault on one stream), AC2 (single vault and omitted flag), AC3 (error on unresolvable name), AC4 (other commands unchanged), DB6 (--types still filters per vault)"
    }
  ]
}

Chunk 4/4

The mechanical funnel already ran (0 findings). Let me complete the manual review.

Manual Review Summary:

For specs/in-progress/047-watch-comma-separated-vault-list.md — this is the spec document itself, not implementation code. Reviewing its quality:

1. Internal consistency: The spec correctly describes the feature as adding getWatchVaults (line 90) as a watch-scoped resolver, separate from getVaults used by all other commands. The createWatchCommand at cli.go:2418 calls getWatchVaults, and getVaults is used by every other command — confirmed by grep.

2. Desired Behavior correctness:

   • getWatchVaults (cli.go:55-85) correctly handles comma-split (strings.Split(*vaultName, ",")), trims whitespace (strings.TrimSpace), skips empty entries (if name == "" { continue }), and errors on no usable names (errors.Errorf with the original value).
   • Leading/trailing commas (",", "a,", ",a") all produce empty segments that are skipped, yielding a non-empty names only if at least one real name existed — but AC3 requires "," specifically to fail loudly. Checking: " , " → split gives " ", trim gives "", skip → len(names)==0 → error with ",". ✓ The error message includes the original flag value, so "," is visible in the error.

3. Test coverage (per the concerns): The tests at watch_test.go:164-240 cover all edge cases: empty segments ("alpha,,beta"), whitespace (" alpha , beta "), leading/trailing commas (",", ",,"), and unresolvable names ("alpha,nope"). Integration tests at integration/watch_test.go verify the full binary.

4. Docs: docs/development-patterns.md:81-89 has the Multi-Vault Pattern section. cli.go:2405-2407 documents --vault personal,trading in the Long description. CHANGELOG has ## Unreleased with the comma-separated vault list phrase.

5. Non-regression: The watch engine (pkg/ops/watch.go) is untouched per spec pins.

No issues found in the spec file or the changed implementation code.

{
  "verdict": "approve",
  "summary": "The spec 047 implementation is correct and complete: getWatchVaults properly parses comma-separated vault names with whitespace trimming and empty-segment skipping, all edge cases (empty segments, whitespace, leading/trailing commas, unresolvable names) are covered by unit and integration tests, the watch command help documents the personal,trading example, docs/development-patterns.md records the Multi-Vault Pattern, CHANGELOG has ## Unreleased with the correct bullet, and no other command's --vault semantics change.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: comma-separated vault list parsing — verify empty segments are handled (e.g. \"vault1,,vault2\")",
      "disposition": "addressed",
      "detail": "getWatchVaults at cli.go:66-70 splits on comma, trims whitespace, skips empty entries. Unit test at watch_test.go:179 covers 'alpha,,beta'."
    },
    {
      "concern": "correctness: whitespace handling around commas — \"vault1, vault2\" vs \"vault1,vault2\"",
      "disposition": "addressed",
      "detail": "getWatchVaults uses strings.TrimSpace on each entry. Unit test at watch_test.go:164 covers ' alpha , beta '."
    },
    {
      "concern": "correctness: trailing/leading comma edge cases — \",vault1\" or \"vault1,\"",
      "disposition": "addressed",
      "detail": "Leading/trailing commas produce empty strings after trim which are skipped by 'if name == \"\" { continue }'. Test at watch_test.go:208 covers ',' and watch_test.go:231 covers ',,'."
    },
    {
      "concern": "tests: unit tests should cover empty segments, whitespace, and leading/trailing commas",
      "disposition": "addressed",
      "detail": "watch_test.go:164-240 has 11 test cases covering all edge cases."
    },
    {
      "concern": "tests: integration tests should exercise multi-vault watch scenarios",
      "disposition": "addressed",
      "detail": "integration/watch_test.go has 7 integration tests covering AC1-AC4 and failure modes."
    }
  ]
}

{"verdict":"approve","reason":"chunked review: all 4 chunks approved","concerns_addressed":[{
      "concern": "correctness: comma-separated vault list parsing — verify empty segments are handled (e.g. \"vault1,,vault2\")",
      "disposition": "addressed",
      "detail": "getWatchVaults at cli.go:67 skips empty entries after TrimSpace; unit test 'skips empty entries between commas' and integration test AC3 cover this"
    },{
      "concern": "correctness: whitespace handling around commas — \"vault1, vault2\" vs \"vault1,vault2\"",
      "disposition": "addressed",
      "detail": "getWatchVaults at cli.go:66 calls strings.TrimSpace on each entry; unit test 'ignores whitespace around each name' and integration test AC1 cover this"
    },{
      "concern": "correctness: trailing/leading comma edge cases — \",vault1\" or \"vault1,\"",
      "disposition": "addressed",
      "detail": "TrimSpace on empty segments produces \"\", which is skipped by the continue; test 'fails when the value is only a comma' and 'fails when the value is only separators' cover this"
    },{
      "concern": "tests: unit tests should cover empty segments, whitespace, and leading/trailing commas",
      "disposition": "addressed",
      "detail": "All edge cases tested in pkg/cli/watch_test.go with dedicated It blocks"
    },{
      "concern": "tests: integration tests should exercise multi-vault watch scenarios",
      "disposition": "addressed",
      "detail": "integration/watch_test.go covers AC1 (both vaults), AC2 (single vault isolation), AC3 (failure modes), AC4 (other commands unaffected), DB6 (--types filter), and missing task directory"
    },{
      "concern": "correctness: comma-separated vault list parsing — verify empty segments are handled",
      "disposition": "addressed",
      "detail": "pkg/cli/watch_test.go:179-193 — test 'skips empty entries between commas' covers \"alpha,,beta\""
    },{
      "concern": "correctness: whitespace handling around commas",
      "disposition": "addressed",
      "detail": "pkg/cli/watch_test.go:164-177 — test 'ignores whitespace around each name' covers \" alpha , beta \""
    },{
      "concern": "correctness: trailing/leading comma edge cases",
      "disposition": "addressed",
      "detail": "pkg/cli/watch_test.go:208-240 — tests cover \",\" (comma only), \" ,,\" (double comma), and \" \" (whitespace only)"
    },{
      "concern": "tests: unit tests should cover empty segments, whitespace, and leading/trailing commas",
      "disposition": "addressed",
      "detail": "12 unit tests in watch_test.go cover all specified edge cases"
    },{
      "concern": "tests: integration tests should exercise multi-vault watch scenarios",
      "disposition": "not-an-issue",
      "detail": "integration/watch_test.go is reviewed in chunk 1/4 — not in scope for this chunk"
    },{
      "concern": "correctness: comma-separated vault list parsing — verify empty segments are handled (e.g. \"vault1,,vault2\")",
      "disposition": "addressed",
      "detail": "cli.go:66-68: empty entries are trimmed then skipped via continue; len(names)==0 check on line 72 returns error if nothing usable remains"
    },{
      "concern": "correctness: whitespace handling around commas — \"vault1, vault2\" vs \"vault1,vault2\"",
      "disposition": "addressed",
      "detail": "cli.go:66: strings.TrimSpace(entry) is called before the empty check, so whitespace on either side of a name is ignored"
    },{
      "concern": "correctness: trailing/leading comma edge cases — \",vault1\" or \"vault1,\"",
      "disposition": "addressed",
      "detail": "cli.go:66-68: both produce \"\" after TrimSpace and are skipped via continue; if all entries skip, len(names)==0 on line 72 triggers error"
    },{
      "concern": "tests: unit tests should cover empty segments, whitespace, and leading/trailing commas",
      "disposition": "addressed",
      "detail": "watch_test.go lines 164-240 cover all these cases: whitespace (line 164), empty entries (line 179), leading/trailing comma (line 208), only separators (line 231)"
    },{
      "concern": "tests: integration tests should exercise multi-vault watch scenarios",
      "disposition": "addressed",
      "detail": "integration/watch_test.go covers AC1 (multi-vault on one stream), AC2 (single vault and omitted flag), AC3 (error on unresolvable name), AC4 (other commands unchanged), DB6 (--types still filters per vault)"
    },{
      "concern": "correctness: comma-separated vault list parsing — verify empty segments are handled (e.g. \"vault1,,vault2\")",
      "disposition": "addressed",
      "detail": "getWatchVaults at cli.go:66-70 splits on comma, trims whitespace, skips empty entries. Unit test at watch_test.go:179 covers 'alpha,,beta'."
    },{
      "concern": "correctness: whitespace handling around commas — \"vault1, vault2\" vs \"vault1,vault2\"",
      "disposition": "addressed",
      "detail": "getWatchVaults uses strings.TrimSpace on each entry. Unit test at watch_test.go:164 covers ' alpha , beta '."
    },{
      "concern": "correctness: trailing/leading comma edge cases — \",vault1\" or \"vault1,\"",
      "disposition": "addressed",
      "detail": "Leading/trailing commas produce empty strings after trim which are skipped by 'if name == \"\" { continue }'. Test at watch_test.go:208 covers ',' and watch_test.go:231 covers ',,'."
    },{
      "concern": "tests: unit tests should cover empty segments, whitespace, and leading/trailing commas",
      "disposition": "addressed",
      "detail": "watch_test.go:164-240 has 11 test cases covering all edge cases."
    },{
      "concern": "tests: integration tests should exercise multi-vault watch scenarios",
      "disposition": "addressed",
      "detail": "integration/watch_test.go has 7 integration tests covering AC1-AC4 and failure modes."
    }]}