fix: address code review findings on the bundle lifecycle
The user-origin marker on replaced mcp.json entries is now sticky: re-records and cross-bundle transfers only upgrade replaced to transferred when the prior record proves bundle origin, so updating a bundle can no longer make uninstall delete a key the user had before the bundle replaced it. Canonical source URLs lowercase only the host, since self-hosted forges treat repository paths as case-sensitive and collapsing distinct repos misdirects updates and uninstalls. git clone invocations pass '--' before the URL so a crafted source cannot be parsed as a git flag. Lifecycle flags (--install, --install-builtins, --update-bundle, --uninstall) and their companions now conflict explicitly instead of first-match dispatch silently dropping actions. --install-from returns as a hidden tombstone that errors with the replacement instead of feeding the flag to the LLM as prompt text. --list-bundles dispatches before config load so a pure read no longer boots MCP servers. write_file_atomic fsyncs before the rename so a crash cannot persist a truncated store. REPL: .uninstall accepts --yes, .install rejects trailing tokens after a category, and .install remote gets a migration hint. Plus polish: host validation rejects '#' and '?', renamed_to no longer serializes null, derived names get a debug assert against the validator, completions share DEFAULT_GIT_HOST, README mentions skills.
This commit is contained in:
+79
-7
@@ -45,6 +45,7 @@ pub(crate) struct FileRecord {
|
||||
pub(crate) struct McpServerRecord {
|
||||
pub(crate) name: String,
|
||||
pub(crate) action: McpAction,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub(crate) renamed_to: Option<String>,
|
||||
/// Hash of the mcp.json entry as written; a later mismatch means the user
|
||||
/// modified it. Absent on records made before entry hashing existed.
|
||||
@@ -299,6 +300,10 @@ impl BundleStore {
|
||||
);
|
||||
}
|
||||
|
||||
debug_assert!(
|
||||
validate_bundle_name(&resolved.name).is_ok(),
|
||||
"derived bundle names must satisfy validate_bundle_name"
|
||||
);
|
||||
Ok(resolved)
|
||||
}
|
||||
|
||||
@@ -391,8 +396,10 @@ impl BundleStore {
|
||||
}
|
||||
|
||||
/// An entry whose key any bundle already owns transfers to `bundle`, and
|
||||
/// its `replaced` action upgrades to `transferred`; plain `replaced`
|
||||
/// marks a pre-existing user entry that uninstall must never delete.
|
||||
/// its `replaced` action upgrades to `transferred` only when the prior
|
||||
/// record proves bundle origin. A prior `replaced` record marks a
|
||||
/// pre-existing user entry that uninstall must never delete, and that
|
||||
/// marker survives re-records and cross-bundle transfers.
|
||||
pub(crate) fn record_mcp_servers(
|
||||
&mut self,
|
||||
bundle: &str,
|
||||
@@ -402,14 +409,21 @@ impl BundleStore {
|
||||
for mut entry in entries {
|
||||
let key = entry.effective_key().to_string();
|
||||
let mut previously_owned = false;
|
||||
let mut prior_user_origin = false;
|
||||
for record in self.bundles.values_mut() {
|
||||
let before = record.mcp_servers.len();
|
||||
record
|
||||
.mcp_servers
|
||||
.retain(|owned| owned.effective_key() != key);
|
||||
record.mcp_servers.retain(|owned| {
|
||||
if owned.effective_key() != key {
|
||||
return true;
|
||||
}
|
||||
if owned.action == McpAction::Replaced {
|
||||
prior_user_origin = true;
|
||||
}
|
||||
false
|
||||
});
|
||||
previously_owned |= record.mcp_servers.len() != before;
|
||||
}
|
||||
if previously_owned && entry.action == McpAction::Replaced {
|
||||
if previously_owned && !prior_user_origin && entry.action == McpAction::Replaced {
|
||||
entry.action = McpAction::Transferred;
|
||||
}
|
||||
self.bundles
|
||||
@@ -852,6 +866,64 @@ mod tests {
|
||||
assert_eq!(servers[0].sha256.as_deref(), Some("deadbeef"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_rerecord_of_own_replaced_entry_stays_replaced() {
|
||||
let dir = TempStoreDir::new("bundles-mcp-sticky-self");
|
||||
let mut store = dir.store();
|
||||
store
|
||||
.upsert_bundle("omc", metadata("https://github.com/x/omc", "abc123"))
|
||||
.unwrap();
|
||||
store
|
||||
.record_mcp_servers(
|
||||
"omc",
|
||||
vec![mcp_record("user-srv", McpAction::Replaced, None)],
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
store
|
||||
.record_mcp_servers(
|
||||
"omc",
|
||||
vec![mcp_record("user-srv", McpAction::Replaced, None)],
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(
|
||||
store.get("omc").unwrap().mcp_servers[0].action,
|
||||
McpAction::Replaced
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_transfer_of_replaced_entry_keeps_user_origin_marker() {
|
||||
let dir = TempStoreDir::new("bundles-mcp-sticky-transfer");
|
||||
let mut store = dir.store();
|
||||
store
|
||||
.upsert_bundle("alpha", metadata("https://github.com/a/alpha", "abc123"))
|
||||
.unwrap();
|
||||
store
|
||||
.upsert_bundle("beta", metadata("https://github.com/b/beta", "def456"))
|
||||
.unwrap();
|
||||
store
|
||||
.record_mcp_servers(
|
||||
"alpha",
|
||||
vec![mcp_record("user-srv", McpAction::Replaced, None)],
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
store
|
||||
.record_mcp_servers(
|
||||
"beta",
|
||||
vec![mcp_record("user-srv", McpAction::Replaced, None)],
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
assert!(store.get("alpha").unwrap().mcp_servers.is_empty());
|
||||
assert_eq!(
|
||||
store.get("beta").unwrap().mcp_servers[0].action,
|
||||
McpAction::Replaced
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_transfer_matches_renamed_entries_by_effective_key() {
|
||||
let dir = TempStoreDir::new("bundles-mcp-renamed");
|
||||
@@ -893,7 +965,7 @@ mod tests {
|
||||
.unwrap();
|
||||
|
||||
let resolved = store
|
||||
.resolve_bundle_name("git@github.com:X/omc.git", None)
|
||||
.resolve_bundle_name("git@github.com:x/omc.git", None)
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(resolved.name, "omc");
|
||||
|
||||
@@ -151,7 +151,7 @@ fn looks_like_remote_source(value: &str) -> bool {
|
||||
}
|
||||
}
|
||||
|
||||
const DEFAULT_GIT_HOST: &str = "github.com";
|
||||
pub(crate) const DEFAULT_GIT_HOST: &str = "github.com";
|
||||
|
||||
fn is_repo_shorthand(value: &str) -> bool {
|
||||
let path = strip_ref_suffix(value);
|
||||
@@ -174,7 +174,7 @@ fn expand_repo_shorthand(value: &str, git_host: Option<&str>) -> Result<String>
|
||||
.or_else(|| raw.strip_prefix("http://"))
|
||||
.unwrap_or(raw)
|
||||
.trim_matches('/');
|
||||
if host.is_empty() || host.contains('/') || host.chars().any(char::is_whitespace) {
|
||||
if host.is_empty() || host.contains(['/', '#', '?']) || host.chars().any(char::is_whitespace) {
|
||||
bail!("invalid --git-host '{raw}': expected a bare host like git.somedomain.com");
|
||||
}
|
||||
Ok(format!("https://{host}/{value}"))
|
||||
@@ -951,12 +951,18 @@ fn clone_to_temp(url: &str, reference: Option<&str>) -> Result<TempRepoDir> {
|
||||
"1".into(),
|
||||
"--branch".into(),
|
||||
r.into(),
|
||||
"--".into(),
|
||||
url.into(),
|
||||
dest_arg,
|
||||
])?;
|
||||
}
|
||||
Some(r) => {
|
||||
run_git(vec!["clone".into(), url.into(), dest_arg.clone()])?;
|
||||
run_git(vec![
|
||||
"clone".into(),
|
||||
"--".into(),
|
||||
url.into(),
|
||||
dest_arg.clone(),
|
||||
])?;
|
||||
run_git(vec!["-C".into(), dest_arg, "checkout".into(), r.into()])?;
|
||||
}
|
||||
None => {
|
||||
@@ -964,6 +970,7 @@ fn clone_to_temp(url: &str, reference: Option<&str>) -> Result<TempRepoDir> {
|
||||
"clone".into(),
|
||||
"--depth".into(),
|
||||
"1".into(),
|
||||
"--".into(),
|
||||
url.into(),
|
||||
dest_arg,
|
||||
])?;
|
||||
@@ -1223,14 +1230,17 @@ fn sanitize_host(host: &str) -> String {
|
||||
.to_string()
|
||||
}
|
||||
|
||||
/// The host compares case-insensitively but the path keeps its case: many
|
||||
/// self-hosted forges treat repository paths as case-sensitive, and collapsing
|
||||
/// distinct repos into one record misdirects updates and uninstalls.
|
||||
pub(crate) fn canonical_source_url(url: &str) -> String {
|
||||
let (host, path) = split_host_and_path(url);
|
||||
let mut path = path.to_ascii_lowercase();
|
||||
if let Some(stripped) = path.strip_suffix(".git")
|
||||
&& !stripped.is_empty()
|
||||
&& !stripped.ends_with('/')
|
||||
{
|
||||
path = stripped.to_string();
|
||||
let mut path = path;
|
||||
if path.to_ascii_lowercase().ends_with(".git") {
|
||||
let stripped = &path[..path.len() - 4];
|
||||
if !stripped.is_empty() && !stripped.ends_with('/') {
|
||||
path.truncate(path.len() - 4);
|
||||
}
|
||||
}
|
||||
let host = host.to_ascii_lowercase();
|
||||
if host.is_empty() {
|
||||
@@ -2837,10 +2847,14 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn canonical_source_url_lowercases_host_and_path() {
|
||||
fn canonical_source_url_lowercases_host_but_not_path() {
|
||||
assert_eq!(
|
||||
canonical_source_url("https://GitHub.COM/X/R.git"),
|
||||
"github.com/x/r"
|
||||
"github.com/X/R"
|
||||
);
|
||||
assert_ne!(
|
||||
canonical_source_url("https://gitlab.example.com/team/Repo"),
|
||||
canonical_source_url("https://gitlab.example.com/team/repo")
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -3501,7 +3501,9 @@ impl RequestContext {
|
||||
InstallFilter::NAMES.iter().map(|s| s.to_string()).collect(),
|
||||
);
|
||||
} else if prev == "--git-host" {
|
||||
values = super::map_completion_values(vec!["github.com".to_string()]);
|
||||
values = super::map_completion_values(vec![
|
||||
super::install_remote::DEFAULT_GIT_HOST.to_string(),
|
||||
]);
|
||||
} else {
|
||||
let has_filter = args.iter().enumerate().any(|(i, a)| {
|
||||
a.starts_with("--filter=") || (*a == "--filter" && i < args.len() - 1)
|
||||
|
||||
Reference in New Issue
Block a user