[Eng] Improve typespec devexp with Update-DevTSPModule

[VeryEarly]

Description

instruction: https://github.com/VeryEarly/azure-powershell/blob/yabo/tsp-client/tools/AzDev/src/Typespec/README.md

• will remove legacy doc before merging
• will update changelog before merging
• will update autorest.powershell to not delete tspconfig.yaml

Mandatory Checklist

• Please choose the target release of Azure PowerShell. (⚠️Target release is a different concept from API readiness. Please click below links for details.)

  • General release
  • Public preview
  • Private preview
  • Engineering build
  • No need for a release

• Check this box to confirm: I have read the Submitting Changes section of CONTRIBUTING.md and reviewed the following information:

• SHOULD update ChangeLog.md file(s) appropriately
  • Update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
  • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
• SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
• SHOULD have proper test coverage for changes in pull request.
• SHOULD NOT adjust version of module manually in pull request

[azure-client-tools-bot-prd]

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

[Copilot]

Pull request overview

This PR introduces a new cmdlet Update-DevTSPModule to the AzDev tooling that automates TypeSpec-based Azure PowerShell module generation. The cmdlet handles downloading TypeSpec configurations from remote GitHub repositories or local paths, merges them with Azure PowerShell-specific configurations, and generates PowerShell modules using the TypeSpec PowerShell emitter.

Key changes:

• Adds comprehensive TypeSpec workflow automation cmdlet with support for remote and local TSP configurations
• Implements YAML serialization support in the YamlHelper service
• Updates build script to clean artifacts directory before rebuilding

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
tools/AzDev/src/Typespec/UpdateTSPModuleCmdlet.cs	New cmdlet implementation handling TSP config resolution, git operations, npm dependency installation, directory copying, and TypeSpec compilation - 759 lines of core logic
tools/AzDev/src/Typespec/README.md	Documentation covering cmdlet usage, terminology, prerequisites, and various usage scenarios including local/remote TSP sources
tools/AzDev/src/Services/YamlHelper.cs	Implements Serialize method to support YAML output generation required by the cmdlet
tools/AzDev/build.ps1	Adds cleanup of artifacts directory before build to ensure clean builds
tools/AzDev/AzDev/AzDev.psd1	Exports the new Update-DevTSPModule cmdlet for module consumers

[Copilot]

Typo in the word "with" - contains extra characters "approved".

Suggested change

	- **Emitter**: The tool responsible for generating PowerShell code from the TypeSpec definition. This cmdlet specifically works witapprovedh the `@azure-tools/typespec-powershell` emitter.
	- **Emitter**: The tool responsible for generating PowerShell code from the TypeSpec definition. This cmdlet specifically works with the `@azure-tools/typespec-powershell` emitter.

[Copilot]

Method name installDependencies doesn't follow C# naming conventions - methods should use PascalCase. Should be InstallDependencies.

Suggested change

	private async Task installDependencies(string workingDirectory)
	private async Task InstallDependencies(string workingDirectory)

[Copilot]

The code doesn't validate that the YAML file contains the expected keys before accessing them with dictionary indexing. If "directory", "repo", or "commit" keys are missing, this will throw a KeyNotFoundException which is less clear than a proper validation error.

Recommendation: Add validation:

Dictionary<string, object> tspLocationPWDContent = YamlHelper.Deserialize<Dictionary<string, object>>(File.ReadAllText(tspLocationPath));

if (!tspLocationPWDContent.ContainsKey("directory") || 
    !tspLocationPWDContent.ContainsKey("repo") || 
    !tspLocationPWDContent.ContainsKey("commit"))
{
    throw new ArgumentException($"Invalid tsp-location.yaml format at {tspLocationPath}. Required keys: directory, repo, commit");
}

[Copilot]

The error message has inconsistent punctuation and grammar. "Azure-PowerShell repository, Or" should not have a comma before "Or", and "Or" should not be capitalized mid-sentence.

Recommendation:

throw new ArgumentException("Unable to determine Azure PowerShell repository root. Please execute this cmdlet in the Azure PowerShell repository, or provide `-RepoRoot` or set it through `Set-DevContext -RepoRoot`.");

Suggested change

	throw new ArgumentException("Unable to determine Azure PowerShell repository root. Please execute this cmdlet in Azure-PowerShell repository, Or please provide `-RepoRoot` or set it through `Set-DevContext -RepoRoot`.");
	throw new ArgumentException("Unable to determine Azure PowerShell repository root. Please execute this cmdlet in the Azure PowerShell repository, or provide `-RepoRoot` or set it through `Set-DevContext -RepoRoot`.");

[Copilot]

The hardcoded .cmd extension makes this code Windows-specific and violates cross-platform compatibility requirements. On Unix-like systems (Linux, macOS), npm and tsp commands don't have the .cmd extension.

Recommendation: Detect the platform and use the appropriate command:

private string FindNPMCommandFromPath(string command)
{
    // Remove .cmd extension for cross-platform compatibility
    string baseCommand = command.Replace(".cmd", "");
    
    if (_npmPath == "" || !File.Exists(_npmPath))
    {
        string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
        string npmPath = pathEnv.Split(Path.PathSeparator)
            .FirstOrDefault(path => path.EndsWith("npm"));
        _npmPath = npmPath;
    }
    
    // On Windows, check for .cmd, .bat, or .exe; on Unix, use the command directly
    string commandPath;
    if (Environment.OSVersion.Platform == PlatformID.Win32NT)
    {
        commandPath = Path.Combine(_npmPath, $"{baseCommand}.cmd");
        if (!File.Exists(commandPath))
            commandPath = Path.Combine(_npmPath, $"{baseCommand}.bat");
        if (!File.Exists(commandPath))
            commandPath = Path.Combine(_npmPath, $"{baseCommand}.exe");
    }
    else
    {
        commandPath = Path.Combine(_npmPath, baseCommand);
    }
    
    if (!File.Exists(commandPath))
    {
        throw new FileNotFoundException($"Command '{baseCommand}' not found in system PATH.");
    }
    return commandPath;
}

[Copilot]

The comment has a typo: "sourcDir" should be "sourceDir". Also, the comment doesn't accurately describe what the method does - it copies the source directory's contents under a subdirectory (with the source directory's name) within the destination directory, not just "put it under" the destination.

Suggested change

	//copy sourcDir and put it under destinationDir
	// Copies the contents of sourceDir into a subdirectory (named after sourceDir) within destinationDir,
	// excluding any files or directories specified in the 'exclude' array.

[Copilot]

The git clone URL and path parameters are constructed using string interpolation without validation or sanitization. This could allow command injection if the repo, path, or commit variables contain malicious characters (e.g., shell metacharacters, quotes).

Recommendation: Validate or escape the parameters before using them in shell commands, or use a library that handles command execution safely. At minimum:

1. Validate repo matches expected format (e.g., "owner/repo-name")
2. Validate commit is a valid 40-character hex SHA
3. Validate path doesn't contain dangerous characters like ;, |, &, etc.

[Copilot]

The git operations use separate commands which is inefficient. Line 362 and 363 both call sparse-checkout - line 363's sparse-checkout add is redundant since line 362's sparse-checkout set already sets the path.

Recommendation: Remove line 363 (sparse-checkout add) as it duplicates line 362's functionality:

await RunCommand("git", $"clone {cloneRepo} {tempDirPath} --no-checkout --filter=tree:0", outDir);
await RunCommand("git", $"sparse-checkout set {path}", tempDirPath);
await RunCommand("git", $"checkout {commit}", tempDirPath);

Suggested change

await RunCommand("git", $"sparse-checkout add {path}", tempDirPath);

[Copilot]

The path construction is incorrect. After calling CopyDirectory(tspLocation, tempDirPath, ...) which copies the directory and its contents under tempDirPath, line 386 attempts to combine tempDirPath, the full tspLocation path, and "tspconfig.yaml". This will create an invalid path.

Looking at the CopyDirectory method (line 398), it creates currentDir = Path.Combine(destinationDir, dir.Name), which means the source directory is copied as a subdirectory with its name, not its full path.

Recommendation: Use only the directory name, not the full path:

string dirName = Path.GetFileName(tspLocation);
string tempTspLocation = Path.Combine(tempDirPath, dirName, "tspconfig.yaml");

Suggested change

	string tempTspLocation = Path.Combine(tempDirPath, tspLocation, "tspconfig.yaml");
	string dirName = Path.GetFileName(tspLocation);
	string tempTspLocation = Path.Combine(tempDirPath, dirName, "tspconfig.yaml");

[Copilot]

The CopyDirectory method only excludes files based on their names (line 404), but doesn't check directory names against the exclude list. This means directories named "tsp-output" or "node_modules" will still be copied recursively.

Recommendation: Add directory exclusion check:

foreach (DirectoryInfo subdir in dirs)
{
    if (exclude != null && Array.Exists(exclude, e => e.Equals(subdir.Name, StringComparison.OrdinalIgnoreCase)))
    {
        continue;
    }
    CopyDirectory(subdir.FullName, currentDir, exclude);
}

Suggested change

	{
	{
	if (exclude != null && Array.Exists(exclude, e => e.Equals(subdir.Name, StringComparison.OrdinalIgnoreCase)))
	{
	continue;
	}

[isra-fel]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 18 comments.

[Copilot]

Potential index range error: Using segment[^1] (index from end) requires C# 8.0+ and .NET Standard 2.1+. If this project targets .NET Standard 2.0 or lower, this will cause a compilation error. Verify the target framework or use:

if (segment[0] == '{' && segment[segment.Length - 1] == '}')
{
    string key = segment.Substring(1, segment.Length - 2);

This is already done correctly on line 466, so line 464 should use the same pattern.

Suggested change

	if (segment[0] == '{' && segment[^1] == '}')
	if (segment[0] == '{' && segment[segment.Length - 1] == '}')

[Copilot]

The comparison _npmPath == "" should use string.IsNullOrEmpty(_npmPath) for consistency with the rest of the codebase and to handle null cases properly.

Suggested change

	if (_npmPath == "" || !File.Exists(_npmPath))
	if (string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))

[Copilot]

The hardcoded npm.cmd command is Windows-specific and will fail on Unix-like systems. See related comment on line 262 for the same issue with tsp.cmd.

[Copilot]

Typo in the comment: "emitter-outout-dir" should be "emitter-output-dir" (missing the 'p').

Suggested change

	// if emitter-outout-dir is not absolute, assume it's relative to RepoRoot
	// if emitter-output-dir is not absolute, assume it's relative to RepoRoot

[Copilot]

Magic string: The tempDirName constant "TempTypeSpecFiles" should be documented with its purpose. Also consider if this name could conflict with user directories or cause issues on case-sensitive file systems.

Suggested change

	/// <summary>
	/// Name of the temporary directory used to store intermediate TypeSpec files during module update operations.
	/// This directory is created in a controlled location and is intended to be cleaned up after use.
	/// The name "TempTypeSpecFiles" was chosen for clarity, but developers should be aware of potential
	/// conflicts if a directory with the same name exists in the target location, especially on case-sensitive file systems.
	/// Consider updating the logic to use a more unique name if this becomes an issue.
	/// </summary>

[Copilot]

The question mark makes the documentation unclear. This should be a statement, not a question. Consider: "- typespec compiler installed: npm install -g @typespec/compiler"

Suggested change

	- **typespec compiler installed?**: `npm install -g @typespec/compiler`
	- **typespec compiler installed**: `npm install -g @typespec/compiler`

[Copilot]

Error message refers to -RemoteRepository but the actual parameter name is -RemoteRepositoryName (line 73). This inconsistency will confuse users trying to follow the error message guidance.

Suggested change

	throw new ArgumentException("Please do not provide `-RemoteDirectory`, `-RemoteCommit`, `-RemoteRepository` or `-RemoteForkName` along with `-TSPLocation`.");
	throw new ArgumentException("Please do not provide `-RemoteDirectory`, `-RemoteCommit`, `-RemoteRepositoryName` or `-RemoteForkName` along with `-TSPLocation`.");

[Copilot]

Magic string: The URI regex pattern is complex and hardcoded. Consider extracting it as a documented constant with an explanation of what it matches. The pattern expects exactly 40 character commit hashes (SHA-1), which could become outdated if GitHub moves to SHA-256 hashes.

Consider:

// Matches GitHub URLs pointing to tspconfig.yaml files with 40-char commit hashes
// Examples: 
// - https://github.com/Azure/azure-rest-api-specs/blob/abc123.../path/tspconfig.yaml
// - https://raw.githubusercontent.com/Azure/azure-rest-api-specs/abc123.../path/tspconfig.yaml
private const string UriRegex = "^https://(?<urlRoot>github|raw.githubusercontent).com/(?<repo>[^/]*/azure-rest-api-specs(-pr)?)/(tree/|blob/)?(?<commit>[0-9a-f]{40})/(?<path>.*)/tspconfig.yaml$";

Suggested change

	private const string UriRegex = "^https://(?<urlRoot>github|raw.githubusercontent).com/(?<repo>[^/]*/azure-rest-api-specs(-pr)?)/(tree/|blob/)?(?<commit>[0-9a-f]{40})/(?<path>.*)/tspconfig.yaml$";
	/// <summary>
	/// Regex pattern to match GitHub URLs pointing to tspconfig.yaml files with 40-character commit hashes (SHA-1).
	///
	/// Examples:
	/// - https://github.com/Azure/azure-rest-api-specs/blob/abc123.../path/tspconfig.yaml
	/// - https://raw.githubusercontent.com/Azure/azure-rest-api-specs/abc123.../path/tspconfig.yaml
	///
	/// Note: The pattern expects exactly 40-character commit hashes (SHA-1). If GitHub moves to SHA-256 hashes (64 characters),
	/// this pattern will need to be updated.
	/// </summary>
	private const string UriRegex = @"^https://(?<urlRoot>github|raw\.githubusercontent)\.com/(?<repo>[^/]*/azure-rest-api-specs(-pr)?)/(tree/|blob/)?(?<commit>[0-9a-f]{40})/(?<path>.*)/tspconfig\.yaml$";

[Copilot]

The static HttpClient instance is good for performance, but it's not being disposed properly. While HttpClient is designed to be long-lived and reused, in a cmdlet scenario where the module may be unloaded, there's no cleanup mechanism. Consider implementing IDisposable or using a different pattern that ensures proper resource cleanup when the module is unloaded.

[VeryEarly]

@copilot see line "using var response = await httpClient.SendAsync(request, cts.Token);" isn't it properly handle?

[Copilot]

The command rm is an alias that may not be available on all systems. Per the repository's cross-platform compatibility guidelines, use the explicit Remove-Item cmdlet for PowerShell 5.1+ compatibility:

Remove-Item -Path "$artifacts/$module" -Recurse -Force -ErrorAction SilentlyContinue