Skip to content

Commit 96fb881

Browse files
harlan-zwharlan-zw
authored
fix(cli): restore direct sources from lockfile (#104)
1 parent 43d33a4 commit 96fb881

4 files changed

Lines changed: 309 additions & 70 deletions

File tree

‎crates/skilld-command/src/lib.rs‎

Lines changed: 92 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@ pub use remote::{
2222
RemoteSourceState, SecretValue, SkilldRemote, Sleeper, ThreadSleeper, TokenProvider,
2323
};
2424
use skilld_core::{
25-
AGENT_TARGETS, AgentTargetId, DomainError, GlobalTargetPath, InstallMode, InstallRequest,
26-
InstallScope, InstallSource, LockedSource, VERSION, select_target_ids,
25+
AGENT_TARGETS, AgentTargetId, DomainError, GlobalTargetPath, InstallMode, InstallOperation,
26+
InstallRequest, InstallScope, InstallSource, LockedSource, VERSION, select_target_ids,
2727
};
2828

2929
#[derive(Debug, Parser)]
@@ -112,9 +112,11 @@ pub trait Host {
112112
"Agent target selection is unavailable on this host",
113113
));
114114
}
115-
let source = request
116-
.source
117-
.ok_or_else(|| CommandError::unsupported_host("lockfile restore is unavailable"))?;
115+
let InstallOperation::Install(source) = request.operation else {
116+
return Err(CommandError::unsupported_host(
117+
"lockfile restore is unavailable",
118+
));
119+
};
118120
self.install(source, request.scope).map(|name| vec![name])
119121
}
120122

@@ -345,19 +347,24 @@ fn dispatch<H: Host>(command: Command, host: &H) -> Result<Vec<String>, CommandE
345347
direct,
346348
} => {
347349
let scope = scope(global);
348-
let source = source
349-
.map(|source| InstallSource::parse(&source))
350-
.map(|source| match (direct, source) {
350+
let operation = match source {
351+
Some(source) => match (direct, InstallSource::parse(&source)) {
351352
(true, InstallSource::Remote(source)) => {
352-
Ok(InstallSource::DirectRemote(source))
353+
InstallOperation::Install(InstallSource::DirectRemote(source))
353354
}
354-
(true, _) => Err(CommandError::input(
355-
"--direct needs an explicit public GitHub Repository selector",
356-
)),
357-
(false, source) => Ok(source),
358-
})
359-
.transpose()?;
360-
if source == Some(InstallSource::BundledSkilld) && scope != InstallScope::Global {
355+
(true, _) => {
356+
return Err(CommandError::input(
357+
"--direct needs an explicit public GitHub Repository selector",
358+
));
359+
}
360+
(false, source) => InstallOperation::Install(source),
361+
},
362+
None if direct => InstallOperation::DirectRestore,
363+
None => InstallOperation::Restore,
364+
};
365+
if operation == InstallOperation::Install(InstallSource::BundledSkilld)
366+
&& scope != InstallScope::Global
367+
{
361368
return Err(CommandError::input(
362369
"install the skilld-maintained Skill with --global",
363370
));
@@ -372,7 +379,7 @@ fn dispatch<H: Host>(command: Command, host: &H) -> Result<Vec<String>, CommandE
372379
.transpose()
373380
.map_err(CommandError::domain)?;
374381
let names = host.install_request(InstallRequest {
375-
source,
382+
operation,
376383
scope,
377384
targets,
378385
mode,
@@ -749,8 +756,13 @@ impl LocalHost {
749756
Ok(name.to_string())
750757
}
751758

752-
fn restore(&self, request: &InstallRequest) -> Result<Vec<String>, CommandError> {
753-
let (targets, known) = self.select_installs(request)?;
759+
fn restore(&self, request: &InstallRequest, direct: bool) -> Result<Vec<String>, CommandError> {
760+
let (targets, known) = if request.targets.is_empty() {
761+
(None, self.known_targets(request.scope)?)
762+
} else {
763+
let (targets, known) = self.select_installs(request)?;
764+
(Some(targets), known)
765+
};
754766
let store = self.store(request.scope);
755767
let names = store.list(&known).map_err(CommandError::store)?;
756768
if names.is_empty() {
@@ -769,24 +781,46 @@ impl LocalHost {
769781
let view = store
770782
.view(&skill_name, &known)
771783
.map_err(CommandError::store)?;
772-
let restored_targets = targets
773-
.iter()
774-
.map(|target| {
775-
let mode = if request.mode.is_some() {
776-
target.mode
777-
} else {
778-
view.skill
779-
.targets
784+
let restored_targets = if let Some(targets) = &targets {
785+
targets
786+
.iter()
787+
.map(|target| {
788+
let mode = if request.mode.is_some() {
789+
target.mode
790+
} else {
791+
view.skill
792+
.targets
793+
.iter()
794+
.find(|locked| locked.agent == target.target.agent)
795+
.map_or(target.mode, |locked| locked.mode)
796+
};
797+
TargetInstall {
798+
target: target.target.clone(),
799+
mode,
800+
}
801+
})
802+
.collect::<Vec<_>>()
803+
} else {
804+
view.skill
805+
.targets
806+
.iter()
807+
.map(|locked| {
808+
known
780809
.iter()
781-
.find(|locked| locked.agent == target.target.agent)
782-
.map_or(target.mode, |locked| locked.mode)
783-
};
784-
TargetInstall {
785-
target: target.target.clone(),
786-
mode,
787-
}
788-
})
789-
.collect::<Vec<_>>();
810+
.find(|target| target.agent == locked.agent)
811+
.cloned()
812+
.map(|target| TargetInstall {
813+
target,
814+
mode: request.mode.unwrap_or(locked.mode),
815+
})
816+
.ok_or_else(|| {
817+
CommandError::domain(DomainError::InvalidTarget(
818+
locked.agent.to_string(),
819+
))
820+
})
821+
})
822+
.collect::<Result<Vec<_>, _>>()?
823+
};
790824
match view.skill.source {
791825
LockedSource::Local { path } => {
792826
let (source, locked_source) =
@@ -805,17 +839,18 @@ impl LocalHost {
805839
LockedSource::Remote {
806840
source, commit_sha, ..
807841
} => {
808-
if !matches!(
809-
view.skill.source_status,
810-
skilld_core::SourceStatus::Verified { .. }
811-
) {
812-
return Err(CommandError {
813-
code: "UNVERIFIED_SOURCE",
814-
message:
815-
"restore an unverified Skill with an explicit --direct install"
816-
.to_owned(),
817-
});
818-
}
842+
let direct = match view.skill.source_status {
843+
skilld_core::SourceStatus::Verified { .. } => false,
844+
skilld_core::SourceStatus::Unverified { .. } if direct => true,
845+
_ => {
846+
return Err(CommandError {
847+
code: "UNVERIFIED_SOURCE",
848+
message:
849+
"run skilld install --direct to restore an unverified Skill"
850+
.to_owned(),
851+
});
852+
}
853+
};
819854
let selector = skilld_core::RemoteSelector::parse(&source)
820855
.map_err(CommandError::remote)?;
821856
let mut exact_source = selector.source().clone();
@@ -830,7 +865,7 @@ impl LocalHost {
830865
};
831866
let prepared = self
832867
.remote_provider()?
833-
.prepare(&exact, false)
868+
.prepare(&exact, direct)
834869
.map_err(CommandError::remote)?;
835870
let staged = materialize_remote(&prepared.files)?;
836871
store
@@ -858,7 +893,7 @@ impl Host for LocalHost {
858893

859894
fn install(&self, source: InstallSource, scope: InstallScope) -> Result<String, CommandError> {
860895
self.install_request(InstallRequest {
861-
source: Some(source),
896+
operation: InstallOperation::Install(source),
862897
scope,
863898
targets: vec![],
864899
mode: None,
@@ -869,8 +904,10 @@ impl Host for LocalHost {
869904
}
870905

871906
fn install_request(&self, request: InstallRequest) -> Result<Vec<String>, CommandError> {
872-
let Some(source) = request.source.clone() else {
873-
return self.restore(&request);
907+
let source = match request.operation.clone() {
908+
InstallOperation::Restore => return self.restore(&request, false),
909+
InstallOperation::DirectRestore => return self.restore(&request, true),
910+
InstallOperation::Install(source) => source,
874911
};
875912
let (targets, known) = self.select_installs(&request)?;
876913
match source {
@@ -1291,7 +1328,10 @@ mod tests {
12911328
}
12921329

12931330
fn install_request(&self, request: InstallRequest) -> Result<Vec<String>, CommandError> {
1294-
assert_eq!(request.source, Some(InstallSource::BundledSkilld));
1331+
assert_eq!(
1332+
request.operation,
1333+
InstallOperation::Install(InstallSource::BundledSkilld)
1334+
);
12951335
assert_eq!(request.scope, InstallScope::Global);
12961336
assert_eq!(request.targets, [AgentTargetId::Codex]);
12971337
Ok(vec!["skilld".to_owned()])

‎crates/skilld-command/tests/agent_targets.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use std::fs;
22
use std::path::Path;
33

44
use skilld_command::{DetectionEnvironment, Host, LocalHost, TargetRoots};
5-
use skilld_core::{AgentTargetId, InstallRequest, InstallScope, InstallSource};
5+
use skilld_core::{AgentTargetId, InstallOperation, InstallRequest, InstallScope, InstallSource};
66

77
fn source(root: &Path) -> std::path::PathBuf {
88
let source = root.join("source/example");
@@ -57,7 +57,7 @@ fn every_project_signal_selects_the_matching_agent_target() {
5757

5858
let names = host
5959
.install_request(InstallRequest {
60-
source: Some(InstallSource::Local(source)),
60+
operation: InstallOperation::Install(InstallSource::Local(source)),
6161
scope: InstallScope::Project,
6262
targets: vec![],
6363
mode: None,
@@ -117,7 +117,7 @@ fn every_runtime_signal_selects_the_matching_agent_target() {
117117
.with_detection_environment(DetectionEnvironment::new([signal.to_owned()]));
118118

119119
host.install_request(InstallRequest {
120-
source: Some(InstallSource::Local(source)),
120+
operation: InstallOperation::Install(InstallSource::Local(source)),
121121
scope: InstallScope::Project,
122122
targets: vec![],
123123
mode: None,
@@ -148,7 +148,7 @@ fn an_existing_global_target_directory_is_detected() {
148148
));
149149

150150
host.install_request(InstallRequest {
151-
source: Some(InstallSource::Local(source)),
151+
operation: InstallOperation::Install(InstallSource::Local(source)),
152152
scope: InstallScope::Global,
153153
targets: vec![],
154154
mode: None,

0 commit comments

Comments
 (0)