Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a consistent --providers flag to multiple SpecStory CLI commands so users can scope operations to one or more provider IDs, backed by a shared ResolveProviderIDs helper to unify argument/flag parsing and validation.
Changes:
- Added
--providers(StringSlice) flag tosync,check,list, andwatch. - Introduced
ResolveProviderIDsinpkg/cmd/utils.goto resolve provider IDs from either a positional arg or--providers. - Refactored “all providers” code paths in
sync,check, andlistto accept an optional provider filter list.
Most important improvements (recommended):
- Ensure
sync -s/--sessionalso honors--providersand enforces the same conflict validation as the non--spath. - Deduplicate provider IDs coming from
--providersto prevent repeated work and duplicate outputs. - Add table-driven unit tests around
ResolveProviderIDsto lock in parsing/validation behavior as more commands adopt it.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| specstory-cli/pkg/cmd/utils.go | Adds shared provider-ID resolution helper used by multiple commands. |
| specstory-cli/pkg/cmd/watch.go | Uses resolved provider IDs for watch mode and adds --providers flag. |
| specstory-cli/pkg/cmd/list.go | Adds --providers flag and plumbs optional provider filtering into “list all”. |
| specstory-cli/pkg/cmd/check.go | Adds --providers flag, updates -c/--command validation, and supports filtered “check all”. |
| specstory-cli/main.go | Adds --providers to sync and updates sync-all flow to accept filtered provider IDs. |
| // syncAllProviders syncs all providers that have activity in the current directory | ||
| func syncAllProviders(registry *factory.Registry, cmd *cobra.Command) error { | ||
| // syncAllProviders syncs all (or a filtered subset of) providers that have activity in the current directory | ||
| // filterIDs, if non-nil, limits which providers are synced; nil means sync all registered providers. |
There was a problem hiding this comment.
The doc comment says filterIDs, if non-nil, limits which providers are synced; nil means sync all, but the implementation uses len(filterIDs) > 0. An empty-but-non-nil slice will behave like nil (sync all), which contradicts the comment. Update the comment to match the actual behavior (e.g., “if non-empty”).
| // filterIDs, if non-nil, limits which providers are synced; nil means sync all registered providers. | |
| // filterIDs, if non-empty, limits which providers are synced; empty or nil means sync all registered providers. |
| providerIDs := registry.ListIDs() | ||
| if len(resolvedIDs) > 0 { | ||
| providerIDs = resolvedIDs | ||
| } |
There was a problem hiding this comment.
resolvedIDs is built to preserve user order (and de-dupe) for --providers, but providerNames later comes from iterating a map of providers, which is nondeterministic. That means the “Watching for activity from …” output (and analytics.SetAgentProviders) won’t reliably reflect the user-specified order. Build providerNames by iterating providerIDs (the slice) to keep output stable and aligned with the flag input.
| // ── Neither specified ─────────────────────────────────────────────────── | ||
| { | ||
| name: "neither arg nor flag returns nil", | ||
| wantIDs: nil, | ||
| }, |
There was a problem hiding this comment.
The “neither arg nor flag returns nil” case doesn’t actually assert that ResolveProviderIDs returns a nil slice; it only compares lengths, so nil and []string{} both pass. If callers rely on the documented “nil means use all providers” contract, consider explicitly checking ids == nil for that test case (and/or distinguishing nil vs empty in assertions).
Supports this issue: #198
This pull request adds consistent support for filtering CLI operations by provider IDs using a new
--providersflag across multiple commands (sync,check,list, andwatch). It introduces a shared utility function to resolve provider IDs from either positional arguments or the new flag, and updates the relevant command logic to use this mechanism. This improves usability by allowing users to target one or more specific providers in a uniform way.Provider filtering support:
Added a
--providersflag to thesync,check,list, andwatchcommands, allowing users to specify one or more provider IDs to limit the operation. [1] [2] [3] [4]Introduced the
ResolveProviderIDsutility function inpkg/cmd/utils.goto resolve provider IDs from either a positional argument or the--providersflag, with validation and error handling for conflicting or invalid input.Command logic refactoring:
sync,check,list, andwatchcommands to use the resolved provider IDs, supporting both single and multiple provider selection, and refactored internal functions (syncAllProviders,checkAllProviders,listAllProviders) to accept optional filtering. [1] [2] [3] [4] [5] [6] [7] [8]Validation and error messaging:
--providersflag, and improved error messages for invalid provider IDs or incorrect flag usage (e.g., ensuring-c/--commandis only used with a single provider). [1] [2]These changes make provider selection more flexible and consistent across commands, improving the CLI's usability and robustness.