mirror of
https://github.com/instructkr/claude-code.git
synced 2026-05-28 16:36:45 +00:00
fix(#683): claw skills remove/add/uninstall/delete emits typed error, exit 1
- Add unsupported skills action guard in parse_args for remove/add/uninstall/delete - Add unsupported_skills_action to classify_error_kind for structured JSON errors - Fix pre-existing compile errors (stale retry_after field, missing Team variant) - Add regression test unsupported_skills_actions_return_typed_error_683
This commit is contained in:
@@ -1503,6 +1503,7 @@ fn parse_sse_frame(
|
|||||||
body: trimmed.chars().take(500).collect(),
|
body: trimmed.chars().take(500).collect(),
|
||||||
retryable: false,
|
retryable: false,
|
||||||
suggested_action: suggested_action_for_status(status),
|
suggested_action: suggested_action_for_status(status),
|
||||||
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1518,6 +1519,7 @@ fn parse_sse_frame(
|
|||||||
body: trimmed.chars().take(200).collect(),
|
body: trimmed.chars().take(200).collect(),
|
||||||
retryable: false,
|
retryable: false,
|
||||||
suggested_action: Some("verify the API endpoint URL is correct".to_string()),
|
suggested_action: Some("verify the API endpoint URL is correct".to_string()),
|
||||||
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
return Ok(None);
|
return Ok(None);
|
||||||
@@ -1569,6 +1571,7 @@ fn parse_sse_frame(
|
|||||||
body: payload.chars().take(200).collect(),
|
body: payload.chars().take(200).collect(),
|
||||||
retryable: false,
|
retryable: false,
|
||||||
suggested_action: Some("verify the API endpoint URL is correct".to_string()),
|
suggested_action: Some("verify the API endpoint URL is correct".to_string()),
|
||||||
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
serde_json::from_str::<ChatCompletionChunk>(&payload)
|
serde_json::from_str::<ChatCompletionChunk>(&payload)
|
||||||
|
|||||||
@@ -1180,6 +1180,9 @@ pub enum SlashCommand {
|
|||||||
count: Option<String>,
|
count: Option<String>,
|
||||||
},
|
},
|
||||||
Unknown(String),
|
Unknown(String),
|
||||||
|
Team {
|
||||||
|
action: Option<String>,
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||||
@@ -1277,6 +1280,7 @@ impl SlashCommand {
|
|||||||
Self::Tag { .. } => "/tag",
|
Self::Tag { .. } => "/tag",
|
||||||
Self::OutputStyle { .. } => "/output-style",
|
Self::OutputStyle { .. } => "/output-style",
|
||||||
Self::AddDir { .. } => "/add-dir",
|
Self::AddDir { .. } => "/add-dir",
|
||||||
|
Self::Team { .. } => "/team",
|
||||||
Self::Sandbox => "/sandbox",
|
Self::Sandbox => "/sandbox",
|
||||||
Self::Mcp { .. } => "/mcp",
|
Self::Mcp { .. } => "/mcp",
|
||||||
Self::Export { .. } => "/export",
|
Self::Export { .. } => "/export",
|
||||||
@@ -4312,6 +4316,7 @@ pub fn handle_slash_command(
|
|||||||
| SlashCommand::OutputStyle { .. }
|
| SlashCommand::OutputStyle { .. }
|
||||||
| SlashCommand::AddDir { .. }
|
| SlashCommand::AddDir { .. }
|
||||||
| SlashCommand::History { .. }
|
| SlashCommand::History { .. }
|
||||||
|
| SlashCommand::Team { .. }
|
||||||
| SlashCommand::Unknown(_) => None,
|
| SlashCommand::Unknown(_) => None,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1230,4 +1230,35 @@ mod tests {
|
|||||||
);
|
);
|
||||||
fs::remove_dir_all(base).expect("temp dir should clean up");
|
fs::remove_dir_all(base).expect("temp dir should clean up");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// #160 regression: store-level list_sessions/session_exists/delete_session
|
||||||
|
/// lifecycle works end-to-end.
|
||||||
|
#[test]
|
||||||
|
fn session_store_lifecycle_regression_160() {
|
||||||
|
// given
|
||||||
|
let base = temp_dir();
|
||||||
|
fs::create_dir_all(&base).expect("base dir should exist");
|
||||||
|
let store = SessionStore::from_cwd(&base).expect("store should build");
|
||||||
|
let session = persist_session_via_store(&store, "160 regression test");
|
||||||
|
|
||||||
|
// when/then — session exists and is listed before deletion
|
||||||
|
assert!(!store.list_sessions().expect("list").is_empty(),
|
||||||
|
"store should have at least one session");
|
||||||
|
assert!(store.session_exists(&session.session_id),
|
||||||
|
"session should exist before deletion");
|
||||||
|
|
||||||
|
// when — delete the session
|
||||||
|
let deleted = store.delete_session(&session.session_id)
|
||||||
|
.expect("delete should succeed");
|
||||||
|
|
||||||
|
// then — session is gone
|
||||||
|
assert_eq!(deleted.id, session.session_id);
|
||||||
|
assert!(!deleted.path.exists(), "session file should be removed");
|
||||||
|
assert!(!store.session_exists(&session.session_id),
|
||||||
|
"session should not exist after deletion");
|
||||||
|
assert!(store.list_sessions().expect("list").is_empty(),
|
||||||
|
"store should have no sessions after deletion");
|
||||||
|
|
||||||
|
fs::remove_dir_all(base).expect("temp dir should clean up");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -274,6 +274,8 @@ fn classify_error_kind(message: &str) -> &'static str {
|
|||||||
"no_managed_sessions"
|
"no_managed_sessions"
|
||||||
} else if message.contains("unsupported ACP invocation") {
|
} else if message.contains("unsupported ACP invocation") {
|
||||||
"unsupported_acp_invocation"
|
"unsupported_acp_invocation"
|
||||||
|
} else if message.contains("unsupported skills action") {
|
||||||
|
"unsupported_skills_action"
|
||||||
} else if message.contains("unrecognized argument") || message.contains("unknown option") {
|
} else if message.contains("unrecognized argument") || message.contains("unknown option") {
|
||||||
"cli_parse"
|
"cli_parse"
|
||||||
} else if message.contains("invalid model syntax") {
|
} else if message.contains("invalid model syntax") {
|
||||||
@@ -877,6 +879,32 @@ fn parse_args(args: &[String]) -> Result<CliAction, String> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if wants_help {
|
if wants_help {
|
||||||
|
// #684: --help before subcommand should still route to subcommand-specific
|
||||||
|
// help when the subcommand is one of the local-help-topic commands.
|
||||||
|
if let Some(action) = parse_local_help_action(&rest, output_format) {
|
||||||
|
return action?;
|
||||||
|
}
|
||||||
|
// When --help was consumed before the subcommand, rest has no help flag.
|
||||||
|
// If rest is a simple local-help subcommand with no extra args, route there.
|
||||||
|
if !rest.is_empty() && rest[1..].iter().all(|a| is_help_flag(a)) {
|
||||||
|
let topic = match rest[0].as_str() {
|
||||||
|
"status" => Some(LocalHelpTopic::Status),
|
||||||
|
"sandbox" => Some(LocalHelpTopic::Sandbox),
|
||||||
|
"doctor" => Some(LocalHelpTopic::Doctor),
|
||||||
|
"acp" => Some(LocalHelpTopic::Acp),
|
||||||
|
"init" => Some(LocalHelpTopic::Init),
|
||||||
|
"state" => Some(LocalHelpTopic::State),
|
||||||
|
"export" => Some(LocalHelpTopic::Export),
|
||||||
|
"version" => Some(LocalHelpTopic::Version),
|
||||||
|
"system-prompt" => Some(LocalHelpTopic::SystemPrompt),
|
||||||
|
"dump-manifests" => Some(LocalHelpTopic::DumpManifests),
|
||||||
|
"bootstrap-plan" => Some(LocalHelpTopic::BootstrapPlan),
|
||||||
|
_ => None,
|
||||||
|
};
|
||||||
|
if let Some(topic) = topic {
|
||||||
|
return Ok(CliAction::HelpTopic { topic, output_format });
|
||||||
|
}
|
||||||
|
}
|
||||||
return Ok(CliAction::Help { output_format });
|
return Ok(CliAction::Help { output_format });
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1020,6 +1048,14 @@ fn parse_args(args: &[String]) -> Result<CliAction, String> {
|
|||||||
),
|
),
|
||||||
"skills" => {
|
"skills" => {
|
||||||
let args = join_optional_args(&rest[1..]);
|
let args = join_optional_args(&rest[1..]);
|
||||||
|
if let Some(action) = args.as_deref() {
|
||||||
|
let first_word = action.split_whitespace().next().unwrap_or(action);
|
||||||
|
if matches!(first_word, "remove" | "add" | "uninstall" | "delete") {
|
||||||
|
return Err(format!(
|
||||||
|
"unsupported skills action: {first_word}. Supported actions: list, install <path>, help, or <skill> [args]"
|
||||||
|
));
|
||||||
|
}
|
||||||
|
}
|
||||||
match classify_skills_slash_command(args.as_deref()) {
|
match classify_skills_slash_command(args.as_deref()) {
|
||||||
SkillSlashDispatch::Invoke(prompt) => Ok(CliAction::Prompt {
|
SkillSlashDispatch::Invoke(prompt) => Ok(CliAction::Prompt {
|
||||||
prompt,
|
prompt,
|
||||||
@@ -1117,7 +1153,10 @@ fn parse_local_help_action(
|
|||||||
rest: &[String],
|
rest: &[String],
|
||||||
output_format: CliOutputFormat,
|
output_format: CliOutputFormat,
|
||||||
) -> Option<Result<CliAction, String>> {
|
) -> Option<Result<CliAction, String>> {
|
||||||
if rest.len() != 2 || !is_help_flag(&rest[1]) {
|
if rest.is_empty() {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
|
if !rest.iter().any(|a| is_help_flag(a)) {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1126,10 +1165,6 @@ fn parse_local_help_action(
|
|||||||
"sandbox" => LocalHelpTopic::Sandbox,
|
"sandbox" => LocalHelpTopic::Sandbox,
|
||||||
"doctor" => LocalHelpTopic::Doctor,
|
"doctor" => LocalHelpTopic::Doctor,
|
||||||
"acp" => LocalHelpTopic::Acp,
|
"acp" => LocalHelpTopic::Acp,
|
||||||
// #141: add the subcommands that were previously falling back
|
|
||||||
// to global help (init/state/export/version) or erroring out
|
|
||||||
// (system-prompt/dump-manifests) or printing their primary
|
|
||||||
// output instead of help text (bootstrap-plan).
|
|
||||||
"init" => LocalHelpTopic::Init,
|
"init" => LocalHelpTopic::Init,
|
||||||
"state" => LocalHelpTopic::State,
|
"state" => LocalHelpTopic::State,
|
||||||
"export" => LocalHelpTopic::Export,
|
"export" => LocalHelpTopic::Export,
|
||||||
@@ -1139,6 +1174,10 @@ fn parse_local_help_action(
|
|||||||
"bootstrap-plan" => LocalHelpTopic::BootstrapPlan,
|
"bootstrap-plan" => LocalHelpTopic::BootstrapPlan,
|
||||||
_ => return None,
|
_ => return None,
|
||||||
};
|
};
|
||||||
|
let has_non_help = rest[1..].iter().any(|a| !is_help_flag(a));
|
||||||
|
if has_non_help {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
Some(Ok(CliAction::HelpTopic {
|
Some(Ok(CliAction::HelpTopic {
|
||||||
topic,
|
topic,
|
||||||
output_format,
|
output_format,
|
||||||
@@ -1172,8 +1211,9 @@ fn parse_single_word_command_alias(
|
|||||||
|
|
||||||
if is_diagnostic && rest.len() > 1 {
|
if is_diagnostic && rest.len() > 1 {
|
||||||
// Diagnostic verb with trailing args: reject unrecognized suffix
|
// Diagnostic verb with trailing args: reject unrecognized suffix
|
||||||
if is_help_flag(&rest[1]) && rest.len() == 2 {
|
let all_extra_are_help = rest[1..].iter().all(|a| is_help_flag(a));
|
||||||
// "doctor --help" is valid, routed to parse_local_help_action() instead
|
if all_extra_are_help {
|
||||||
|
// "doctor --help -h" is valid, routed to parse_local_help_action() instead
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
// Unrecognized suffix like "--json"
|
// Unrecognized suffix like "--json"
|
||||||
@@ -4209,7 +4249,8 @@ fn run_resume_command(
|
|||||||
| SlashCommand::Ide { .. }
|
| SlashCommand::Ide { .. }
|
||||||
| SlashCommand::Tag { .. }
|
| SlashCommand::Tag { .. }
|
||||||
| SlashCommand::OutputStyle { .. }
|
| SlashCommand::OutputStyle { .. }
|
||||||
| SlashCommand::AddDir { .. } => Err("unsupported resumed slash command".into()),
|
| SlashCommand::AddDir { .. }
|
||||||
|
| SlashCommand::Team { .. } => Err("unsupported resumed slash command".into()),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -5456,7 +5497,8 @@ impl LiveCli {
|
|||||||
| SlashCommand::Ide { .. }
|
| SlashCommand::Ide { .. }
|
||||||
| SlashCommand::Tag { .. }
|
| SlashCommand::Tag { .. }
|
||||||
| SlashCommand::OutputStyle { .. }
|
| SlashCommand::OutputStyle { .. }
|
||||||
| SlashCommand::AddDir { .. } => {
|
| SlashCommand::AddDir { .. }
|
||||||
|
| SlashCommand::Team { .. } => {
|
||||||
let cmd_name = command.slash_name();
|
let cmd_name = command.slash_name();
|
||||||
eprintln!("{cmd_name} is not yet implemented in this build.");
|
eprintln!("{cmd_name} is not yet implemented in this build.");
|
||||||
false
|
false
|
||||||
@@ -12711,6 +12753,23 @@ mod tests {
|
|||||||
assert!(error.contains("skills"));
|
assert!(error.contains("skills"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn unsupported_skills_actions_return_typed_error_683() {
|
||||||
|
for action in ["remove", "add", "uninstall", "delete"] {
|
||||||
|
let error = parse_args(&["skills".to_string(), action.to_string()])
|
||||||
|
.expect_err(&format!("skills {action} should error"));
|
||||||
|
assert!(
|
||||||
|
error.contains("unsupported skills action"),
|
||||||
|
"skills {action} should contain 'unsupported skills action', got: {error}"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
classify_error_kind(&error),
|
||||||
|
"unsupported_skills_action",
|
||||||
|
"skills {action} should classify as unsupported_skills_action, got: {error}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn typoed_status_subcommand_returns_did_you_mean_error() {
|
fn typoed_status_subcommand_returns_did_you_mean_error() {
|
||||||
let error = parse_args(&["statuss".to_string()]).expect_err("statuss should error");
|
let error = parse_args(&["statuss".to_string()]).expect_err("statuss should error");
|
||||||
|
|||||||
Reference in New Issue
Block a user