From 74277a84c488cff38700f1f44d575341b7423b30 Mon Sep 17 00:00:00 2001 From: "Claude Opus 5.5" Date: Wed, 23 Sep 2026 04:01:48 +0000 Subject: [PATCH] fix(cli): reject flags that were accepted and silently ignored Follows the d661d88e rule: validate explicit user input. - `index rebuild --max-concurrent N` was parsed and never read; the core reindex seam is sequential. Remove the flag so passing it is an "unknown argument" error instead of a no-op. The e2e test now asserts the rejection. - `skill search --embedding` only applies to local search, and remote is the default scope, so `skill search "q" --embedding true` quietly ran a remote search. Reject --embedding without --local, like --paths. The QA smoke plan had three local-search steps missing --local, and they were silently exercising remote search; add --local. Co-Authored-By: Claude Opus 5.5 --- crates/fastskill-cli/src/commands/reindex.rs | 35 ------------------ crates/fastskill-cli/src/commands/search.rs | 30 ++++++++++++++-- .../fastskill-cli/src/utils/reindex_utils.rs | 1 - docs/qa/smoke-test-plan.md | 6 ++-- tests/cli/reindex_e2e_tests.rs | 36 +++++-------------- ...pshot_helpers__reindex_max_concurrent.snap | 7 ---- ...snapshot_helpers__search_command_help.snap | 2 +- webdocs/cli-reference/reindex-command.mdx | 8 ----- webdocs/cli-reference/search-command.mdx | 2 +- 9 files changed, 42 insertions(+), 85 deletions(-) delete mode 100644 tests/cli/snapshots/cli_tests__cli__snapshot_helpers__reindex_max_concurrent.snap diff --git a/crates/fastskill-cli/src/commands/reindex.rs b/crates/fastskill-cli/src/commands/reindex.rs index e9c2fa12..cd6637c1 100644 --- a/crates/fastskill-cli/src/commands/reindex.rs +++ b/crates/fastskill-cli/src/commands/reindex.rs @@ -62,14 +62,6 @@ pub struct ReindexArgs { /// Force re-indexing of all skills (ignore existing hashes) pub force: bool, - /// Maximum number of concurrent embedding requests - /// - /// NOTE (core-seam gap): the core `reindex` seam processes skills - /// sequentially and takes no concurrency parameter, so this is currently - /// accepted but has no effect. Kept for CLI/arg compatibility. - #[allow(dead_code)] - pub max_concurrent: usize, - /// Show progress bars and processing details pub progress: bool, @@ -108,17 +100,6 @@ impl IntoCommandSpec for ReindexArgs { default: None, ..Default::default() }, - ArgSpec { - name: "max-concurrent", - long: Some("max-concurrent"), - short: None, - help: "Maximum concurrent embedding requests", - kind: ArgKind::Option, - value_type: ArgValueType::Int, - cardinality: Cardinality::Optional, - default: Some(ArgValue::Int(5)), - ..Default::default() - }, ArgSpec { name: "progress", long: Some("progress"), @@ -158,16 +139,6 @@ impl FromArgValueMap for ReindexArgs { } }), force: matches!(map.get("force"), Some(ArgValue::Bool(true))), - max_concurrent: map - .get("max-concurrent") - .and_then(|v| { - if let ArgValue::Int(n) = v { - Some(*n as usize) - } else { - None - } - }) - .unwrap_or(5), progress: matches!(map.get("progress"), Some(ArgValue::Bool(true))), no_progress: matches!(map.get("no-progress"), Some(ArgValue::Bool(true))), } @@ -344,7 +315,6 @@ Test skill content"#, let args = ReindexArgs { skills_dir: None, force: false, - max_concurrent: 5, progress: false, no_progress: false, }; @@ -378,7 +348,6 @@ Test skill content"#, let args = ReindexArgs { skills_dir: Some(nonexistent_dir), force: false, - max_concurrent: 5, progress: false, no_progress: false, }; @@ -413,7 +382,6 @@ Test skill content"#, let args = ReindexArgs { skills_dir: Some(skills_dir), force: true, - max_concurrent: 5, progress: false, no_progress: false, }; @@ -458,7 +426,6 @@ Test skill content"#, let args = ReindexArgs { skills_dir: Some(skills_dir.clone()), force: false, - max_concurrent: 2, progress: true, no_progress: false, }; @@ -469,7 +436,6 @@ Test skill content"#, let force_args = ReindexArgs { skills_dir: Some(skills_dir), force: true, - max_concurrent: 2, progress: false, no_progress: true, }; @@ -494,7 +460,6 @@ Test skill content"#, let args = ReindexArgs { skills_dir: None, force: false, - max_concurrent: 5, progress: true, no_progress: true, }; diff --git a/crates/fastskill-cli/src/commands/search.rs b/crates/fastskill-cli/src/commands/search.rs index ae335298..c504db5c 100644 --- a/crates/fastskill-cli/src/commands/search.rs +++ b/crates/fastskill-cli/src/commands/search.rs @@ -43,7 +43,7 @@ pub struct SearchArgs { pub json: bool, /// Use embedding search: true, false, or auto - /// Only applies to local search; ignored for remote search + /// Only valid with --local; rejected for remote search pub embedding: Option, /// Skills directory path (overrides default discovery) @@ -151,7 +151,7 @@ impl IntoCommandSpec for SearchArgs { name: "embedding", long: Some("embedding"), short: None, - help: "Use embedding search: true, false, or auto", + help: "Use embedding search (requires --local): true, false, or auto", kind: ArgKind::Option, value_type: ArgValueType::String, cardinality: Cardinality::Optional, @@ -412,6 +412,12 @@ fn validate_search_args(args: &SearchArgs) -> CliResult<()> { )); } + if args.embedding.is_some() && !args.local { + return Err(CliError::Config( + "--embedding requires --local; remote search does not use embeddings".to_string(), + )); + } + if args.paths && !args.local { return Err(CliError::Config( "--paths requires --local; use 'fastskill skill search --local --paths '" @@ -653,6 +659,26 @@ mod tests { assert!(result.is_ok()); } + #[test] + fn test_validate_search_args_rejects_embedding_for_remote() { + let args = SearchArgs { + query: "test".to_string(), + local: false, + remote: false, + repository: None, + limit: 10, + format: None, + json: false, + embedding: Some("true".to_string()), + skills_dir: None, + paths: false, + content: None, + }; + + let error = validate_search_args(&args).unwrap_err(); + assert!(error.to_string().contains("--embedding requires --local")); + } + #[test] fn test_validate_search_args_valid_remote_with_repository() { let args = SearchArgs { diff --git a/crates/fastskill-cli/src/utils/reindex_utils.rs b/crates/fastskill-cli/src/utils/reindex_utils.rs index 2fcb6c68..7c0af9d6 100644 --- a/crates/fastskill-cli/src/utils/reindex_utils.rs +++ b/crates/fastskill-cli/src/utils/reindex_utils.rs @@ -114,7 +114,6 @@ pub async fn maybe_auto_reindex( let args = crate::commands::reindex::ReindexArgs { skills_dir: None, force: false, - max_concurrent: 5, progress: false, no_progress: true, }; diff --git a/docs/qa/smoke-test-plan.md b/docs/qa/smoke-test-plan.md index 35178af4..9d04e8fc 100644 --- a/docs/qa/smoke-test-plan.md +++ b/docs/qa/smoke-test-plan.md @@ -369,7 +369,7 @@ embedding_model = "text-embedding-3-small" |---|---|---|---|---|---| | 8.1 | `fastskill cli doctor` | `embedding_config` = Pass ("configuration found"); `api_key` = Pass ("OPENAI_API_KEY is set"). | 🤖 | doctor | ☐P ☐F ☐G | | 8.2 | `fastskill --skills-dir "$SKILLS_DIR" index rebuild` | Actually builds the index (not skipped), exit 0, reports a count. | 🤖 | configuration | ☐P ☐F ☐G | -| 8.3 | `fastskill --skills-dir "$SKILLS_DIR" skill search "edit documents" --embedding true` | Returns semantically-ranked results, exit 0. | 🧑 | search-command | ☐P ☐F ☐G | +| 8.3 | `fastskill --skills-dir "$SKILLS_DIR" skill search --local "edit documents" --embedding true` | Returns semantically-ranked results, exit 0. | 🧑 | search-command | ☐P ☐F ☐G | | 8.4 | `fastskill --skills-dir "$SKILLS_DIR" analysis matrix` | Pairwise similarity output, exit 0. | 🤖 | analysis | ☐P ☐F ☐G | | 8.5 | `fastskill --skills-dir "$SKILLS_DIR" analysis duplicates` | Duplicate/near-duplicate pairs (or none), exit 0. | 🤖 | analysis | ☐P ☐F ☐G | | 8.6 | `fastskill --skills-dir "$SKILLS_DIR" analysis cluster --num-clusters 2` | Clusters the skills, exit 0. | 🤖 | analysis | ☐P ☐F ☐G | @@ -386,13 +386,13 @@ embedding_model = "" | # | Command | Expected observable | Mode | Src | Result | |---|---|---|---|---|---| | 8.8 | `fastskill --skills-dir "$SKILLS_DIR" index rebuild` | Reindex succeeds **against the gateway** — confirm via gateway logs that the request hit `/v1/embeddings`. **This is the real proof the override works.** | 🧑 | embedding.rs | ☐P ☐F ☐G | -| 8.9 | `fastskill --skills-dir "$SKILLS_DIR" skill search "edit documents" --embedding true` | Semantic results via the gateway, exit 0. | 🧑 | — | ☐P ☐F ☐G | +| 8.9 | `fastskill --skills-dir "$SKILLS_DIR" skill search --local "edit documents" --embedding true` | Semantic results via the gateway, exit 0. | 🧑 | — | ☐P ☐F ☐G | **Variant C — graceful skip (no provider).** Temporarily unset the key: | # | Command | Expected observable | Mode | Src | Result | |---|---|---|---|---|---| | 8.10 | `OPENAI_API_KEY= fastskill --skills-dir "$SKILLS_DIR" index rebuild` | "Reindex skipped: … Run 'fastskill cli doctor' …", **exit 0**. | 🧑 | reindex | ☐P ☐F ☐G | -| 8.11 | `OPENAI_API_KEY= fastskill --skills-dir "$SKILLS_DIR" skill search "x"` (no `--embedding`) | Falls back to text/fuzzy search, exit 0. | 🧑 | search local | ☐P ☐F ☐G | +| 8.11 | `OPENAI_API_KEY= fastskill --skills-dir "$SKILLS_DIR" skill search --local "x"` (no `--embedding`) | Falls back to text/fuzzy search, exit 0. | 🧑 | search local | ☐P ☐F ☐G | ### Section 9 — Eval diff --git a/tests/cli/reindex_e2e_tests.rs b/tests/cli/reindex_e2e_tests.rs index 40d3263b..71e79f13 100644 --- a/tests/cli/reindex_e2e_tests.rs +++ b/tests/cli/reindex_e2e_tests.rs @@ -131,39 +131,21 @@ embedding_model = "text-embedding-3-small" assert_snapshot_with_settings("reindex_force", &result.stdout, &cli_snapshot_settings()); } +/// `--max-concurrent` was accepted and silently ignored (indexing is +/// sequential); it is now an unknown argument so callers learn it did nothing. #[test] -fn test_reindex_max_concurrent() { +fn test_reindex_rejects_max_concurrent() { let temp_dir = TempDir::new().unwrap(); - let skills_dir = temp_dir.path().join(".skills"); - fs::create_dir_all(&skills_dir).unwrap(); - - // Create config for reindex - let config_content = r#"[dependencies] - -[tool.fastskill] -skills_directory = ".skills" - -[tool.fastskill.embedding] -openai_base_url = "https://api.openai.com/v1" -embedding_model = "text-embedding-3-small" -"#; - fs::write(temp_dir.path().join("skill-project.toml"), config_content).unwrap(); - - // Set OPENAI_API_KEY to avoid config requirement - let env_vars = vec![("OPENAI_API_KEY", "test-key")]; - let result = run_fastskill_command_with_env( + let result = run_fastskill_command( &["index", "rebuild", "--max-concurrent", "2"], - &env_vars, Some(temp_dir.path()), ); - assert!(result.success); - // Should succeed with concurrency limit - - assert_snapshot_with_settings( - "reindex_max_concurrent", - &result.stdout, - &cli_snapshot_settings(), + assert!(!result.success, "--max-concurrent must be rejected"); + assert!( + result.stderr.contains("unknown argument"), + "stderr: {}", + result.stderr ); } diff --git a/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__reindex_max_concurrent.snap b/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__reindex_max_concurrent.snap deleted file mode 100644 index 6901dce7..00000000 --- a/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__reindex_max_concurrent.snap +++ /dev/null @@ -1,7 +0,0 @@ ---- -source: crates/fastskill-cli/../../tests/cli/snapshot_helpers.rs -assertion_line: 262 -expression: normalized ---- -Found 0 skills to process -No skills found in [TEMP_DIR] diff --git a/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__search_command_help.snap b/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__search_command_help.snap index 94a9ec4f..b078925b 100644 --- a/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__search_command_help.snap +++ b/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__search_command_help.snap @@ -21,7 +21,7 @@ Options: -l, --limit Maximum number of results [default: 10] -f, --format Output format: table, json, grid, xml --json Shorthand for --format json - --embedding Use embedding search: true, false, or auto + --embedding Use embedding search (requires --local): true, false, or auto --paths Output resolved file paths instead of search results (--local only) --content Content to include in JSON output: none, preview, full (--local --paths only) diff --git a/webdocs/cli-reference/reindex-command.mdx b/webdocs/cli-reference/reindex-command.mdx index f0fcafcb..d26a789a 100644 --- a/webdocs/cli-reference/reindex-command.mdx +++ b/webdocs/cli-reference/reindex-command.mdx @@ -46,14 +46,6 @@ fastskill index rebuild --skills-dir .claude/skills/ --force - When embeddings seem outdated - To rebuild corrupted index -### --max-concurrent `<NUMBER>` - - -This flag is currently a **no-op**. It is accepted for compatibility but has no effect: -`index rebuild` embeds skills **sequentially**, one at a time. Passing a value does not change -indexing speed or the number of concurrent API requests. - - ## Configuration Requirements ### 1. Embedding Configuration diff --git a/webdocs/cli-reference/search-command.mdx b/webdocs/cli-reference/search-command.mdx index 0b96b4e1..1a6cc446 100644 --- a/webdocs/cli-reference/search-command.mdx +++ b/webdocs/cli-reference/search-command.mdx @@ -29,7 +29,7 @@ to search only installed skills. | `-l, --limit ` | Return between 1 and 1,000 results. | `10` | | `-f, --format ` | Render `table`, `json`, `grid`, or `xml`. | `table` | | `--json` | Shorthand for `--format json`; mutually exclusive with `--format`. | `false` | -| `--embedding ` | Local search mode: `true`, `false`, or `auto`. | `auto` | +| `--embedding ` | Local search mode: `true`, `false`, or `auto`. Requires `--local`; rejected for remote search. | `auto` | | `--paths` | Return resolved paths as JSON; requires `--local`. | `false` | | `--content ` | Include `none`, `preview`, or `full`; requires `--local --paths`. | `none` | | `--skills-dir ` | Override the installation directory for this invocation. | project setting |