Skip to content

Conversation

@VeryEarly
Copy link
Collaborator

Description

Mandatory Checklist

  • 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

Copilot AI review requested due to automatic review settings December 17, 2025 05:09
@azure-client-tools-bot-prd
Copy link

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

@github-actions
Copy link

‼️ DO NOT MERGE THIS PR ‼️
This PR was labeled "Do Not Merge" because it contains code change that cannot be merged. Please contact the reviewer for more information.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request introduces a new TypeSpec (TSP) flow for generating Azure PowerShell modules directly from TypeSpec definitions. The PR is marked as "[DO NOT REVIEW]" and labeled as a pipeline test, indicating this is experimental infrastructure work to support TypeSpec-based code generation alongside the existing AutoRest flow.

Key changes:

  • Adds a new New-DevTSPModule cmdlet to the AzDev tooling that handles TypeSpec-based module generation, including fetching TSP configurations from remote or local sources, merging configurations, and invoking the TypeSpec compiler
  • Integrates the new TSP flow into the existing build pipeline by modifying PrepareAutorestModule.ps1 and BuildScripts.psm1 to detect and use TSP-based generation when tsp-location.yaml is present
  • Implements YAML serialization support in YamlHelper.cs to enable configuration file generation

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 27 comments.

Show a summary per file
File Description
tools/AzDev/src/Typespec/NewTSPModuleCmdlet.cs New 784-line cmdlet implementing TypeSpec module generation with remote/local TSP fetching, configuration merging, and compilation orchestration
tools/AzDev/src/Typespec/README.md Comprehensive documentation for the new cmdlet including terminology, prerequisites, usage examples, and migration guidance
tools/AzDev/src/Services/YamlHelper.cs Implements YAML serialization by adding Serialize method and ISerializer instance
tools/AzDev/AzDev/AzDev.psd1 Exports the new New-DevTSPModule cmdlet from the AzDev module
tools/AzDev/build.ps1 Refactors build script to use Join-Path for cross-platform compatibility and adds clean build logic
tools/BuildScripts/PrepareAutorestModule.ps1 Adds AzDev module build and import logic to make New-DevTSPModule available during module preparation
tools/BuildScripts/BuildScripts.psm1 Integrates TSP flow by calling New-DevTSPModule when tsp-location.yaml is detected instead of AutoRest
src/DependencyMap/DependencyMap/ChangeLog.md Adds test changelog entry
src/DependencyMap/DependencyMap.Autorest/tspconfig.yaml New TypeSpec configuration file with PowerShell emitter options and directives for the DependencyMap module
src/DependencyMap/DependencyMap.Autorest/tsp-location.yaml Updates remote repository reference from Azure to VeryEarly fork with new commit hash
src/DependencyMap/DependencyMap.Autorest/generate-info.json Updates generation ID to track this new generation

Copy-Item "$PSScriptRoot/$module/*" "$artifacts/$module" -Recurse -Force
if (Test-Path $moduleOut) { Remove-Item $moduleOut -Recurse -Force }
dotnet publish (Join-Path $PSScriptRoot "src") --sc -o (Join-Path $moduleOut "bin")
Copy-Item (Join-Path $PSScriptRoot $module "*") $moduleOut -Recurse -Force
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script uses Join-Path correctly for cross-platform compatibility, which is good. However, verify that the dotnet publish and Copy-Item commands work correctly on Unix-like systems (Linux, macOS), especially the wildcard pattern in Copy-Item on line 7. Consider testing this script on non-Windows platforms to ensure full compatibility.

Suggested change
Copy-Item (Join-Path $PSScriptRoot $module "*") $moduleOut -Recurse -Force
Get-ChildItem -Path (Join-Path $PSScriptRoot $module) | Copy-Item -Destination $moduleOut -Recurse -Force

Copilot uses AI. Check for mistakes.
// ----------------------------------------------------------------------------------

using AzDev.Models;
using NuGet.Protocol.Plugins;
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The using statement 'NuGet.Protocol.Plugins' is imported but doesn't appear to be used in this file. Consider removing unused imports to keep the code clean and reduce compilation dependencies.

Suggested change
using NuGet.Protocol.Plugins;

Copilot uses AI. Check for mistakes.
Comment on lines +459 to +464
Console.WriteLine(e.Data);
};
process.ErrorDataReceived += (sender, e) =>
{
if (!string.IsNullOrEmpty(e.Data))
Console.Error.WriteLine(e.Data);
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cmdlet uses Console.WriteLine and Console.Error.WriteLine directly instead of PowerShell's standard WriteVerbose, WriteWarning, or WriteError methods. This bypasses PowerShell's standard stream handling and preference variables. Replace Console.WriteLine with WriteVerbose or WriteInformation, and Console.Error.WriteLine with WriteError for consistency with PowerShell conventions.

Suggested change
Console.WriteLine(e.Data);
};
process.ErrorDataReceived += (sender, e) =>
{
if (!string.IsNullOrEmpty(e.Data))
Console.Error.WriteLine(e.Data);
{
// Route standard output through PowerShell Information stream
WriteInformation(e.Data, new[] { "NewTSPModuleCmdlet" });
}
};
process.ErrorDataReceived += (sender, e) =>
{
if (!string.IsNullOrEmpty(e.Data))
{
// Route standard error through PowerShell Error stream
var errorRecord = new ErrorRecord(
new InvalidOperationException(e.Data),
"ExternalCommandError",
ErrorCategory.NotSpecified,
targetObject: null);
WriteError(errorRecord);
}

Copilot uses AI. Check for mistakes.
Comment on lines +627 to +629
catch (Exception ex)
{
throw new Exception($"Failed to download tspconfig from {uri}, {ex.Message}", ex);
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code catches a general Exception and wraps it, but the inner exception message is redundant since it's already included in the outer exception. Consider either removing the inner exception message from the outer message string, or simplifying the error handling to avoid duplicate information in the exception chain.

Copilot uses AI. Check for mistakes.
Comment on lines 304 to 316
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
if (!File.Exists(commandPath))
{

throw new FileNotFoundException($"Command '{command}' not found in system PATH.");
}
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition checks for string.IsNullOrEmpty(_npmPath) but _npmPath is assigned a value inside the if block. If the FirstOrDefault returns null, _npmPath will be null, and on the next call, the condition will still be true, causing the PATH to be searched again unnecessarily. The logic should check if _npmPath is null or empty after the assignment and only proceed if a valid path was found. Additionally, if no npm directory is found, this should throw an appropriate error rather than continuing with a null _npmPath.

Suggested change
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
if (!File.Exists(commandPath))
{
throw new FileNotFoundException($"Command '{command}' not found in system PATH.");
}
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd" : "";
if (string.IsNullOrEmpty(_npmPath) || !Directory.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
string npmPath = pathEnv
.Split(Path.PathSeparator)
.FirstOrDefault(path => path.EndsWith("npm", StringComparison.OrdinalIgnoreCase) && Directory.Exists(path));
if (string.IsNullOrEmpty(npmPath))
{
throw new DirectoryNotFoundException("Unable to locate an npm directory in the system PATH.");
}
_npmPath = npmPath;
}
string commandPath = Path.Combine(_npmPath, command + commandSuffix);
if (!File.Exists(commandPath))
{
throw new FileNotFoundException($"Command '{command}' not found in system PATH.");
}

Copilot uses AI. Check for mistakes.
Comment on lines +39 to +40
- **Sample remote TSP Location without PowerShell emitter option**: https://github.com/VeryEarly/azure-rest-api-specs/blob/yabo/test-tspconfig/specification/azuredependencymap/DependencyMap.Management/tspconfig.yaml
- **Sample AsPSConfig with PowerShell emitter option**: https://github.com/VeryEarly/azure-powershell/blob/yabo/tsp-client-bugbash/src/DependencyMap/DependencyMap.Autorest/tspconfig.yaml
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sample URLs point to personal forks (VeryEarly) rather than official Azure repositories. For documentation intended for broader use, consider using examples from the official Azure repositories, or clearly label these as example/test URLs that users should replace with their own.

Suggested change
- **Sample remote TSP Location without PowerShell emitter option**: https://github.com/VeryEarly/azure-rest-api-specs/blob/yabo/test-tspconfig/specification/azuredependencymap/DependencyMap.Management/tspconfig.yaml
- **Sample AsPSConfig with PowerShell emitter option**: https://github.com/VeryEarly/azure-powershell/blob/yabo/tsp-client-bugbash/src/DependencyMap/DependencyMap.Autorest/tspconfig.yaml
- **Sample remote TSP Location without PowerShell emitter option**: https://github.com/Azure/azure-rest-api-specs/blob/29e9e3ca1a1bccba66a6cf092dbc317c639989b1/specification/azuredependencymap/DependencyMap.Management/tspconfig.yaml
- **Sample AzPSConfig with PowerShell emitter option**: https://github.com/Azure/azure-powershell/blob/main/path-to-your-module/tspconfig.yaml (replace with the path to your own tspconfig.yaml)

Copilot uses AI. Check for mistakes.
- Additional information about change #1
-->
## Upcoming Release
* Pipeline test
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ChangeLog entry "Pipeline test" is not user-facing and doesn't follow the required format. According to the ChangeLog.md guidelines, entries should describe changes from the user's perspective and explain what changed and how it affects their usage. This appears to be a test entry that should be removed or replaced with a proper description of the changes being introduced.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +700 to +712
Console.WriteLine($"No ChildConfig provided, use {parentConfigPath}");
return parent;
}
string childConfig = GetTSPConfig(childConfigPath);
// Validate and deserialize child config
if (string.IsNullOrWhiteSpace(childConfig) || !YamlHelper.TryDeserialize<IDictionary<object, object>>(childConfig, out IDictionary<object, object> child))
{
throw new ArgumentException("Invalid child TSP config: " + childConfig, nameof(childConfig));
}

Console.WriteLine("Performing deep merge for parent: " + parentConfigPath + " and child: " + childConfigPath);
var mergedConfig = MergeNestedObjectIteratively(parent, child);
Console.WriteLine("TSP config merge completed successfully");
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to other Console.WriteLine usage in this file, use PowerShell's WriteVerbose or WriteInformation methods instead for consistency with PowerShell cmdlet conventions.

Copilot uses AI. Check for mistakes.
Comment on lines 304 to 311
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name 'commandSuffix' could be more descriptive. Consider renaming it to 'commandFileExtension' or 'platformCommandExtension' to make it clearer that this represents the file extension for commands on different platforms.

Suggested change
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
string commandFileExtension = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd" : "";
if (string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
string commandPath = Path.Combine(_npmPath, command + commandFileExtension);

Copilot uses AI. Check for mistakes.
Comment on lines 94 to 97
$tspLocationPath = Join-Path $GenerateDirectory "tsp-location.yaml"
if (Test-Path $tspLocationPath) {
tsp-client update >> $GenerateLog
# Not good practice to do this, this requires 'PrepareAutorestModule.ps1' to prepare AzDev and pipeline to install tsp compiler'
New-DevTSPModule >> $GenerateLog
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new call to New-DevTSPModule in the pipeline context processes tsp-location.yaml, whose directory field can point to any local file path and is fully attacker-controlled via the PR. Inside New-DevTSPModule, this path is read and, if the file is not a valid TSP YAML dictionary, its entire contents are concatenated into an exception message (Invalid parent TSP config: ...), which will be written into the build logs, allowing a malicious contributor to exfiltrate arbitrary readable files (including potential secrets) from the build agent. To mitigate this, strictly validate or restrict the allowed paths from tsp-location.yaml (e.g., only under the repo root), and avoid echoing raw file contents in error messages for invalid configs.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 17, 2025 10:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 21 comments.


## Prerequisite
- **node version >= 20**
- **typespec compiler installed?**: `npm install -g @typespec/compiler`
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in URL - "typespec compiler installed?" should be phrased as a statement rather than a question. Change to "TypeSpec compiler must be installed" for clarity.

Copilot uses AI. Check for mistakes.
New-DevTSPModule
```

### Use tsp-location.yaml with updated commit and fork When last time generated from remote
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The section references "When last time generated from remote" which is grammatically incorrect. It should read "When last generation was from remote" to match standard English grammar and maintain consistency with line 15 which uses similar phrasing.

Copilot uses AI. Check for mistakes.
throw new InvalidOperationException($"Failed to prepare temporary directory [{tempDirPath}]: {ex.Message}", ex);
}
string cloneRepo = $"https://github.com/{repo}.git";
await RunCommand("git", $"clone {cloneRepo} {tempDirPath} --no-checkout --filter=tree:0", outDir);
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The git commands use --filter=tree:0 for partial clones, which is a relatively new Git feature (Git 2.19+). Consider adding validation to check the Git version or document the minimum required Git version in the README.md to avoid confusing errors when users have older Git installations.

Copilot uses AI. Check for mistakes.
Comment on lines +380 to +381
await RunCommand("git", $"sparse-checkout set {path}", tempDirPath);
await RunCommand("git", $"sparse-checkout add {path}", tempDirPath);
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance concern: The git sparse-checkout commands on lines 380-381 are redundant. The "git sparse-checkout set" command already configures sparse checkout, so "git sparse-checkout add" immediately after will just re-add the same path. Remove line 381 as it's unnecessary.

Copilot uses AI. Check for mistakes.

private string FindNPMCommandFromPath(string command)
{
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The platform check is incorrect. PlatformID.Win32NT is obsolete and doesn't reliably detect Windows on modern .NET. Use RuntimeInformation.IsOSPlatform(OSPlatform.Windows) from System.Runtime.InteropServices for cross-platform detection.

Copilot uses AI. Check for mistakes.
$tspLocationPath = Join-Path $GenerateDirectory "tsp-location.yaml"
if (Test-Path $tspLocationPath) {
tsp-client update >> $GenerateLog
# Not good practice to do this, this requires 'PrepareAutorestModule.ps1' to prepare AzDev
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment indicates this is not good practice but provides no context on why or what the alternative should be. Comments like this should either explain the issue and planned resolution, or link to a tracking issue. Better yet, implement the proper solution instead of leaving a note about bad practice.

Copilot uses AI. Check for mistakes.
[Cmdlet("New", "DevTSPModule")]
public class NewTSPModuleCmdlet : DevCmdletBase
{
private static readonly HttpClient httpClient = new HttpClient();
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HttpClient is static and shared across all cmdlet invocations, but the timeout is set per request using CancellationTokenSource. This is good, but consider that HttpClient timeout should be set to Timeout.InfiniteTimeSpan when using per-request timeouts to avoid conflicts. Also, the static HttpClient should ideally be disposed when the module unloads, though this is a minor concern for long-running tools.

Copilot uses AI. Check for mistakes.
Comment on lines +197 to +204
try
{
context = ContextProvider.LoadContext();
}
catch
{
context = null;
}
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty catch block silently suppresses all exceptions when loading context. At minimum, use WriteVerbose or WriteDebug to log that context loading failed and why. This makes debugging configuration issues much harder.

Copilot uses AI. Check for mistakes.
{
throw new InvalidOperationException($"Failed to prepare temporary directory [{tempDirPath}]: {ex.Message}", ex);
}
CopyDirectory(tspLocation, tempDirPath, ["tsp-output", "node_modules"]);
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The collection expression syntax with square brackets for array literals requires C# 12. Consider using traditional array initialization "new string[] { "tsp-output", "node_modules" }" for better compatibility, or ensure the project explicitly targets C# 12+.

Copilot uses AI. Check for mistakes.
}
}

private object MergeTSPConfig(string parentConfigPath, string childConfigPath)
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method MergeTSPConfig returns type 'object' but always returns an IDictionary. This should return IDictionary<object, object> for type safety and to match the actual usage pattern where the result is immediately cast to Dictionary<object, object>.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 18, 2025 03:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment on lines +201 to +202
catch
{
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The catch block that loads context catches all exceptions but doesn't log or handle them specifically. While the code continues with context = null, silently swallowing exceptions could hide configuration issues. Consider logging a warning when context loading fails so users are aware of the issue.

Suggested change
catch
{
catch (Exception ex)
{
WriteWarning($"Failed to load development context. Continuing without context. Details: {ex.Message}");

Copilot uses AI. Check for mistakes.
{
Scheme = "https",
Host = "raw.githubusercontent.com",
Path = $"{RemoteRepositoryName ?? "Azure/azure-rest-api-specs"}/{RemoteCommit ?? "main"}/{RemoteDirectory ?? ""}/tspconfig.yaml"
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string interpolation in UriBuilder uses null-coalescing operators but the fallback values may not make sense. For example, RemoteCommit defaults to "main" which is a branch name, not a commit hash. This could cause issues when the URI is used later. Consider validating that required values are present before constructing the URI, or document why "main" is an acceptable fallback for a commit parameter.

Suggested change
Path = $"{RemoteRepositoryName ?? "Azure/azure-rest-api-specs"}/{RemoteCommit ?? "main"}/{RemoteDirectory ?? ""}/tspconfig.yaml"
Path = $"{RemoteRepositoryName}/{RemoteCommit}/{RemoteDirectory}/tspconfig.yaml"

Copilot uses AI. Check for mistakes.
Comment on lines 459 to 670
Console.WriteLine(e.Data);
};
process.ErrorDataReceived += (sender, e) =>
{
if (!string.IsNullOrEmpty(e.Data))
Console.Error.WriteLine(e.Data);
};

process.Start();
process.BeginOutputReadLine();
process.BeginErrorReadLine();
await process.WaitForExitAsync();
if (process.ExitCode != 0)
{
throw new InvalidOperationException($"Command '{command} {arguments}' failed with exit code {process.ExitCode}");
}
}
}

private string TryResolveDirFromTSPConfig(Dictionary<object, object> option, string dir)
{
if (string.IsNullOrEmpty(dir))
{
return null;
}
StringBuilder resolvedDir = new StringBuilder();
string[] segments = dir.Split('/', '\\', StringSplitOptions.RemoveEmptyEntries);
for (int i = 0; i < segments.Length; i++)
{
string segment = segments[i];
if (segment[0] == '{' && segment[^1] == '}')
{
string key = segment.Substring(1, segment.Length - 2);
segment = option.ContainsKey(key) ? (string)option[key] : string.Empty;
}
if (string.IsNullOrEmpty(segment))
{
continue;
}
resolvedDir.Append(segment);
if (i < segments.Length - 1)
{
resolvedDir.Append(Path.DirectorySeparatorChar);
}

}
return resolvedDir.ToString();
}

private bool IsRoot(string path) => Directory.Exists(Path.Combine(path, ".azure-pipelines")) &&
Directory.Exists(Path.Combine(path, "src")) &&
Directory.Exists(Path.Combine(path, "generated")) &&
Directory.Exists(Path.Combine(path, ".github"));

private string GetRepoRoot((DevContext, string, string) repoInfo)
{
(DevContext context, string repoRoot, string currentPath) = repoInfo;
if (!string.IsNullOrEmpty(repoRoot))
{
if (!Directory.Exists(repoRoot) || !IsRoot(repoRoot))
{
throw new ArgumentException($"The provided RepoRoot [{repoRoot}] is not a valid Azure PowerShell repository root.");
}
return repoRoot;
}
if (context != null && !string.IsNullOrEmpty(context.AzurePowerShellRepositoryRoot) && Directory.Exists(context.AzurePowerShellRepositoryRoot))
{
return context.AzurePowerShellRepositoryRoot;
}
string potentialRoot = currentPath;
while (!string.IsNullOrEmpty(potentialRoot) && !IsRoot(potentialRoot))
{
potentialRoot = Path.GetDirectoryName(potentialRoot);
}
if (string.IsNullOrEmpty(potentialRoot))
{
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`.");
}
return potentialRoot;
}

private string ConstructTSPConfigUriFromTSPLocation(string tspLocationPath, (string, string, string, string) remoteInfo)
{
Dictionary<string, object> tspLocationPWDContent = YamlHelper.Deserialize<Dictionary<string, object>>(File.ReadAllText(tspLocationPath));
//if tspconfig emitted previously was from local, only record the absolute directory name
if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"]))
{
if (remoteInfo != (null, null, null, null))
{
throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`.");
}
return (string)tspLocationPWDContent["directory"];
}
(string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo;
//otherwise it was from remote, construct its url
string repo = !string.IsNullOrEmpty(RemoteForkName) ? $"{RemoteForkName}/azure-rest-api-specs" : (!string.IsNullOrEmpty(RemoteRepositoryName) ? RemoteRepositoryName : (string)tspLocationPWDContent["repo"]);
string commit = !string.IsNullOrEmpty(RemoteCommit) ? RemoteCommit : (string)tspLocationPWDContent["commit"];
string directory = !string.IsNullOrEmpty(RemoteDirectory) ? RemoteDirectory : (string)tspLocationPWDContent["directory"];
UriBuilder uriBuilder = new UriBuilder
{
Scheme = "https",
Host = "raw.githubusercontent.com",
Path = $"{repo}/{commit}/{directory}/tspconfig.yaml"
};
return uriBuilder.ToString();
}

private bool IsRemoteUri(string uri) => Uri.TryCreate(uri, UriKind.Absolute, out Uri uriResult) && (uriResult.Scheme == Uri.UriSchemeHttp || uriResult.Scheme == Uri.UriSchemeHttps);

private string GetTSPConfig(string uri) => IsRemoteUri(uri) ? GetTSPConfigRemote(uri).GetAwaiter().GetResult() : GetTSPConfigLocal(uri).GetAwaiter().GetResult();

private async Task<string> GetTSPConfigRemote(string uri)
{
// Validate URI
if (string.IsNullOrWhiteSpace(uri))
{
throw new ArgumentException("URI cannot be null or empty", nameof(uri));
}

if (!Uri.TryCreate(uri, UriKind.Absolute, out Uri validatedUri))
{
throw new ArgumentException($"Invalid URI format: {uri}", nameof(uri));
}

// Ensure HTTPS for security
if (validatedUri.Scheme != Uri.UriSchemeHttps && validatedUri.Scheme != Uri.UriSchemeHttp)
{
throw new ArgumentException($"Only HTTP and HTTPS URIs are supported: {uri}", nameof(uri));
}

Console.WriteLine($"Downloading TSP config from: {uri}");

// Prepare request and timeout
using var request = new HttpRequestMessage(HttpMethod.Get, uri);
request.Headers.Add("User-Agent", "AzDev-TSPConfig/1.0");

using var cts = new System.Threading.CancellationTokenSource(TimeSpan.FromMinutes(2));

try
{
// Send request and get response
using var response = await httpClient.SendAsync(request, cts.Token);

// Check response status
if (!response.IsSuccessStatusCode)
{
throw new HttpRequestException($"Failed to download TSP config. Status: {response.StatusCode}, Reason: {response.ReasonPhrase}");
}

// Read and validate content
var content = await response.Content.ReadAsStringAsync();

if (string.IsNullOrWhiteSpace(content))
{
throw new InvalidOperationException("Downloaded TSP config content is empty");
}

Console.WriteLine($"Successfully downloaded TSP config ({content.Length} characters)");
return content;
}
catch (TaskCanceledException ex) when (ex.InnerException is TimeoutException || ex.CancellationToken.IsCancellationRequested)
{
throw new TimeoutException($"Timeout occurred while downloading TSP config from {uri}", ex);
}
catch (HttpRequestException ex)
{
throw new InvalidOperationException($"Network error occurred while downloading TSP config: {ex.Message}", ex);
}
catch (Exception ex)
{
throw new Exception($"Failed to download tspconfig from {uri}, {ex.Message}", ex);
}
}

private async Task<string> GetTSPConfigLocal(string uri)
{
// Validate uri
if (string.IsNullOrWhiteSpace(uri))
{
throw new ArgumentException("URI cannot be null or empty", nameof(uri));
}

// Normalize and validate the path
string normalizedPath;
try
{
normalizedPath = Path.GetFullPath(uri);
}
catch (Exception ex)
{
throw new ArgumentException($"Invalid file path: {uri}", nameof(uri), ex);
}

// Check if file exists
if (!File.Exists(normalizedPath))
{
throw new FileNotFoundException($"TSP config file not found: {normalizedPath}", normalizedPath);
}

Console.WriteLine($"Reading TSP config from local file: {normalizedPath}");

try
{
// Read file content asynchronously
var content = await File.ReadAllTextAsync(normalizedPath);

if (string.IsNullOrWhiteSpace(content))
{
throw new InvalidOperationException($"TSP config file is empty: {normalizedPath}");
}

Console.WriteLine($"Successfully read TSP config from local file ({content.Length} characters)");
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Console.WriteLine and Console.Error.WriteLine in a PowerShell cmdlet is not best practice. These write directly to stdout/stderr and bypass PowerShell's output streams. Use WriteObject, WriteVerbose, WriteWarning, WriteError, or WriteDebug instead, which properly integrate with PowerShell's output system and allow users to redirect or suppress output as needed.

Copilot uses AI. Check for mistakes.
Comment on lines 1 to +7
$module = 'AzDev'
$artifacts = "$PSScriptRoot/../../artifacts"
$artifacts = Join-Path $PSScriptRoot ".." ".." "artifacts"
$moduleOut = Join-Path $artifacts $module

dotnet publish $PSScriptRoot/src --sc -o "$artifacts/$module/bin"
Copy-Item "$PSScriptRoot/$module/*" "$artifacts/$module" -Recurse -Force
if (Test-Path $moduleOut) { Remove-Item $moduleOut -Recurse -Force }
dotnet publish (Join-Path $PSScriptRoot "src") --sc -o (Join-Path $moduleOut "bin")
Copy-Item (Join-Path $PSScriptRoot $module "*") $moduleOut -Recurse -Force
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable naming is inconsistent with PowerShell conventions. PowerShell variables typically use PascalCase for clarity, but some variables here use lowercase (like $module, $artifacts, $moduleOut). While not incorrect, using consistent PascalCase (like $Module, $Artifacts, $ModuleOut) would be more conventional and improve readability.

Copilot uses AI. Check for mistakes.
Comment on lines 543 to 556
//if tspconfig emitted previously was from local, only record the absolute directory name
if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"]))
{
if (remoteInfo != (null, null, null, null))
{
throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`.");
}
return (string)tspLocationPWDContent["directory"];
}
(string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo;
//otherwise it was from remote, construct its url
string repo = !string.IsNullOrEmpty(RemoteForkName) ? $"{RemoteForkName}/azure-rest-api-specs" : (!string.IsNullOrEmpty(RemoteRepositoryName) ? RemoteRepositoryName : (string)tspLocationPWDContent["repo"]);
string commit = !string.IsNullOrEmpty(RemoteCommit) ? RemoteCommit : (string)tspLocationPWDContent["commit"];
string directory = !string.IsNullOrEmpty(RemoteDirectory) ? RemoteDirectory : (string)tspLocationPWDContent["directory"];
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file check uses File.Exists on tspLocationPWDContent["directory"] without validating that the dictionary contains that key or that the value is a non-null string. This could throw a KeyNotFoundException or NullReferenceException. Add proper validation before accessing dictionary values.

Suggested change
//if tspconfig emitted previously was from local, only record the absolute directory name
if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"]))
{
if (remoteInfo != (null, null, null, null))
{
throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`.");
}
return (string)tspLocationPWDContent["directory"];
}
(string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo;
//otherwise it was from remote, construct its url
string repo = !string.IsNullOrEmpty(RemoteForkName) ? $"{RemoteForkName}/azure-rest-api-specs" : (!string.IsNullOrEmpty(RemoteRepositoryName) ? RemoteRepositoryName : (string)tspLocationPWDContent["repo"]);
string commit = !string.IsNullOrEmpty(RemoteCommit) ? RemoteCommit : (string)tspLocationPWDContent["commit"];
string directory = !string.IsNullOrEmpty(RemoteDirectory) ? RemoteDirectory : (string)tspLocationPWDContent["directory"];
// Safely extract expected values from the deserialized content
string directoryFromFile = null;
if (tspLocationPWDContent != null &&
tspLocationPWDContent.TryGetValue("directory", out object directoryObj) &&
directoryObj is string directoryStr)
{
directoryFromFile = directoryStr;
}
string repoFromFile = null;
if (tspLocationPWDContent != null &&
tspLocationPWDContent.TryGetValue("repo", out object repoObj) &&
repoObj is string repoStr)
{
repoFromFile = repoStr;
}
string commitFromFile = null;
if (tspLocationPWDContent != null &&
tspLocationPWDContent.TryGetValue("commit", out object commitObj) &&
commitObj is string commitStr)
{
commitFromFile = commitStr;
}
// if tspconfig emitted previously was from local, only record the absolute directory name
if (!string.IsNullOrEmpty(directoryFromFile) &&
File.Exists(directoryFromFile) &&
string.IsNullOrEmpty(repoFromFile) &&
string.IsNullOrEmpty(commitFromFile))
{
if (remoteInfo != (null, null, null, null))
{
throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`.");
}
return directoryFromFile;
}
(string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo;
// otherwise it was from remote, construct its url
string repo = !string.IsNullOrEmpty(RemoteForkName)
? $"{RemoteForkName}/azure-rest-api-specs"
: (!string.IsNullOrEmpty(RemoteRepositoryName)
? RemoteRepositoryName
: repoFromFile);
string commit = !string.IsNullOrEmpty(RemoteCommit) ? RemoteCommit : commitFromFile;
string directory = !string.IsNullOrEmpty(RemoteDirectory) ? RemoteDirectory : directoryFromFile;
if (string.IsNullOrEmpty(repo) || string.IsNullOrEmpty(commit) || string.IsNullOrEmpty(directory))
{
throw new ArgumentException("The TSP location file does not contain valid 'repo', 'commit', or 'directory' information and no remote override was provided.");
}

Copilot uses AI. Check for mistakes.

private string FindNPMCommandFromPath(string command)
{
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded command suffix logic only handles Windows. On Windows, the suffix should be ".cmd", but on Unix-like systems it should be empty. However, the condition uses Platform == Win32NT which won't properly distinguish when to use no suffix. The ternary operator should use an empty string as the true branch result for Windows and empty for non-Windows, but the logic appears inverted. On Windows, npm commands are typically "npm.cmd", "tsp.cmd", etc., while on Unix they have no extension.

Copilot uses AI. Check for mistakes.
//if tspconfig emitted previously was from local, only record the absolute directory name
if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"]))
{
if (remoteInfo != (null, null, null, null))
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tuple equality check using "!= (null, null, null, null)" is unreliable in C#. Tuple equality compares each element, but this pattern doesn't clearly express the intent. Consider using a more explicit check like checking if any of the tuple elements is not null: "remoteInfo.Item1 != null || remoteInfo.Item2 != null || remoteInfo.Item3 != null || remoteInfo.Item4 != null".

Suggested change
if (remoteInfo != (null, null, null, null))
if (remoteInfo.Item1 != null || remoteInfo.Item2 != null || remoteInfo.Item3 != null || remoteInfo.Item4 != null)

Copilot uses AI. Check for mistakes.
throw new ArgumentException($"Invalid URI format: {uri}", nameof(uri));
}

// Ensure HTTPS for security
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTTP and HTTPS validation comment says "Ensure HTTPS for security" but the code actually allows both HTTP and HTTPS. If security is a concern, the code should reject HTTP URLs and only allow HTTPS. Either remove the misleading comment or enforce HTTPS-only.

Suggested change
// Ensure HTTPS for security
// Ensure the URI uses a supported scheme (HTTP or HTTPS)

Copilot uses AI. Check for mistakes.
Write-Warning "Cannot find generated directory: $generatedDirectory"
}

if (-Not (Get-Module -Name "AzDev")) {
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PrepareAutorestModule.ps1 script doesn't include a shebang line for cross-platform compatibility. Add "#!/usr/bin/env pwsh" at the top to ensure the script can be executed directly on Unix-like systems.

Copilot generated this review using guidance from repository custom instructions.
Copilot AI review requested due to automatic review settings December 18, 2025 03:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment on lines 308 to 320
Console.WriteLine.WriteLine($"##########DEBUG: looking for command: {command} ##########");
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Console.WriteLine.WriteLine($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine.WriteLine($"##########DEBUG: looking for path: {pathEnv} ##########");
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
Console.WriteLine.WriteLine($"##########DEBUG: looking for npm path: {_npmPath} ##########");
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
Console.WriteLine.WriteLine($"##########DEBUG: Found npm command path: {commandPath} ##########");
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debug logging statements should use the PowerShell WriteVerbose or WriteDebug cmdlets instead of Console.WriteLine for consistency with PowerShell cmdlet conventions. Also, consider adding a Debug parameter to control these messages.

Suggested change
Console.WriteLine.WriteLine($"##########DEBUG: looking for command: {command} ##########");
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Console.WriteLine.WriteLine($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine.WriteLine($"##########DEBUG: looking for path: {pathEnv} ##########");
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
Console.WriteLine.WriteLine($"##########DEBUG: looking for npm path: {_npmPath} ##########");
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
Console.WriteLine.WriteLine($"##########DEBUG: Found npm command path: {commandPath} ##########");
WriteDebug($"##########DEBUG: looking for command: {command} ##########");
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
WriteDebug($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
WriteDebug($"##########DEBUG: looking for path: {pathEnv} ##########");
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
WriteDebug($"##########DEBUG: looking for npm path: {_npmPath} ##########");
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
WriteDebug($"##########DEBUG: Found npm command path: {commandPath} ##########");

Copilot uses AI. Check for mistakes.
Comment on lines 308 to 320
Console.WriteLine.WriteLine($"##########DEBUG: looking for command: {command} ##########");
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Console.WriteLine.WriteLine($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine.WriteLine($"##########DEBUG: looking for path: {pathEnv} ##########");
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
Console.WriteLine.WriteLine($"##########DEBUG: looking for npm path: {_npmPath} ##########");
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
Console.WriteLine.WriteLine($"##########DEBUG: Found npm command path: {commandPath} ##########");
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The syntax Console.WriteLine.WriteLine is incorrect. It should be just Console.WriteLine. This appears in multiple debug statements and will cause compilation errors.

Suggested change
Console.WriteLine.WriteLine($"##########DEBUG: looking for command: {command} ##########");
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Console.WriteLine.WriteLine($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine.WriteLine($"##########DEBUG: looking for path: {pathEnv} ##########");
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
Console.WriteLine.WriteLine($"##########DEBUG: looking for npm path: {_npmPath} ##########");
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
Console.WriteLine.WriteLine($"##########DEBUG: Found npm command path: {commandPath} ##########");
Console.WriteLine($"##########DEBUG: looking for command: {command} ##########");
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Console.WriteLine($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine($"##########DEBUG: looking for path: {pathEnv} ##########");
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
Console.WriteLine($"##########DEBUG: looking for npm path: {_npmPath} ##########");
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
Console.WriteLine($"##########DEBUG: Found npm command path: {commandPath} ##########");

Copilot uses AI. Check for mistakes.
Comment on lines +582 to +586
// Ensure HTTPS for security
if (validatedUri.Scheme != Uri.UriSchemeHttps && validatedUri.Scheme != Uri.UriSchemeHttp)
{
throw new ArgumentException($"Only HTTP and HTTPS URIs are supported: {uri}", nameof(uri));
}
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on line 582 says "Ensure HTTPS for security" but the code allows HTTP connections on line 583. If security is a concern, HTTP should not be allowed. Either remove HTTP support or update the comment to accurately reflect that both HTTP and HTTPS are permitted.

Copilot uses AI. Check for mistakes.
Comment on lines +210 to +212
Dictionary<object, object> mergedTspConfig = (Dictionary<object, object>)MergeTSPConfig(TSPLocation, AzPSConfig);
Dictionary<object, object> options = (Dictionary<object, object>)mergedTspConfig["options"];
Dictionary<object, object> option = (Dictionary<object, object>)options[emitterName];
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 210-212 perform unsafe casts without null or type checks. If the TSP config structure doesn't match expectations (e.g., missing "options" key, or the emitter is not configured), these casts will throw InvalidCastException or KeyNotFoundException. Add proper validation and provide clear error messages.

Suggested change
Dictionary<object, object> mergedTspConfig = (Dictionary<object, object>)MergeTSPConfig(TSPLocation, AzPSConfig);
Dictionary<object, object> options = (Dictionary<object, object>)mergedTspConfig["options"];
Dictionary<object, object> option = (Dictionary<object, object>)options[emitterName];
var mergedTspConfigObject = MergeTSPConfig(TSPLocation, AzPSConfig);
if (mergedTspConfigObject is not Dictionary<object, object> mergedTspConfig)
{
throw new ArgumentException($"Merged TSP config for '{TSPLocation}' is not in the expected format.");
}
if (!mergedTspConfig.TryGetValue("options", out var optionsObject) ||
optionsObject is not Dictionary<object, object> options)
{
throw new ArgumentException($"TSP config '{TSPLocation}' does not contain a valid 'options' section.");
}
if (!options.TryGetValue(emitterName, out var emitterOptionObject) ||
emitterOptionObject is not Dictionary<object, object> option)
{
throw new ArgumentException($"TSP config '{TSPLocation}' does not define valid options for emitter '{emitterName}'.");
}

Copilot uses AI. Check for mistakes.
Comment on lines 308 to 326
Console.WriteLine.WriteLine($"##########DEBUG: looking for command: {command} ##########");
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Console.WriteLine.WriteLine($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine.WriteLine($"##########DEBUG: looking for path: {pathEnv} ##########");
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
Console.WriteLine.WriteLine($"##########DEBUG: looking for npm path: {_npmPath} ##########");
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
Console.WriteLine.WriteLine($"##########DEBUG: Found npm command path: {commandPath} ##########");
if (!File.Exists(commandPath))
{

throw new FileNotFoundException($"Command '{command}' not found in system PATH.");
}
return commandPath;
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NPM command finding logic looks for paths ending with "npm" which could match unintended paths. A more robust approach would be to search for the actual npm executable (npm or npm.cmd on Windows) in each PATH directory.

Suggested change
Console.WriteLine.WriteLine($"##########DEBUG: looking for command: {command} ##########");
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Console.WriteLine.WriteLine($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine.WriteLine($"##########DEBUG: looking for path: {pathEnv} ##########");
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
Console.WriteLine.WriteLine($"##########DEBUG: looking for npm path: {_npmPath} ##########");
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
Console.WriteLine.WriteLine($"##########DEBUG: Found npm command path: {commandPath} ##########");
if (!File.Exists(commandPath))
{
throw new FileNotFoundException($"Command '{command}' not found in system PATH.");
}
return commandPath;
Console.WriteLine($"##########DEBUG: looking for command: {command} ##########");
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd" : string.Empty;
Console.WriteLine($"##########DEBUG: using suffix: {commandSuffix} ##########");
// Reuse cached directory if it is still valid and contains the command
if (!string.IsNullOrEmpty(_npmPath))
{
var cachedCommandPath = Path.Combine(_npmPath, command + commandSuffix);
if (File.Exists(cachedCommandPath))
{
Console.WriteLine($"##########DEBUG: using cached npm path: {cachedCommandPath} ##########");
return cachedCommandPath;
}
}
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine($"##########DEBUG: PATH: {pathEnv} ##########");
foreach (var rawEntry in pathEnv.Split(Path.PathSeparator))
{
if (string.IsNullOrWhiteSpace(rawEntry))
{
continue;
}
var entry = rawEntry.Trim();
// Case 1: PATH entry is a directory
if (Directory.Exists(entry))
{
var candidate = Path.Combine(entry, command + commandSuffix);
Console.WriteLine($"##########DEBUG: probing directory candidate: {candidate} ##########");
if (File.Exists(candidate))
{
_npmPath = entry;
Console.WriteLine($"##########DEBUG: Found npm command path in directory: {candidate} ##########");
return candidate;
}
}
else if (File.Exists(entry))
{
// Case 2: PATH entry is a full path to an executable
var fileNameWithoutExt = Path.GetFileNameWithoutExtension(entry);
if (string.Equals(fileNameWithoutExt, command, StringComparison.OrdinalIgnoreCase))
{
_npmPath = Path.GetDirectoryName(entry);
Console.WriteLine($"##########DEBUG: Found npm command path as file entry: {entry} ##########");
return entry;
}
}
}
throw new FileNotFoundException($"Command '{command}' not found in system PATH.");

Copilot uses AI. Check for mistakes.

private string PrepareTSPFromLocal(string tspLocation, string outDir)
{
tspLocation = Path.GetDirectoryName(tspLocation);
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 387 calls Path.GetDirectoryName which can return null if the path is a root directory or invalid. This could cause a NullReferenceException when passed to DirectoryInfo constructor on line 409. Add a null check after this call.

Suggested change
tspLocation = Path.GetDirectoryName(tspLocation);
var directory = Path.GetDirectoryName(tspLocation);
if (string.IsNullOrEmpty(directory))
{
throw new ArgumentException($"The provided TSP location [{tspLocation}] is not a valid path to a file within a directory.");
}
tspLocation = directory;

Copilot uses AI. Check for mistakes.
Comment on lines 278 to 282
if (!File.Exists(Path.Combine(workingDirectory, "package.json")))
{
throw new FileNotFoundException($"package.json not found in {workingDirectory}");
}
RunCommand(FindNPMCommandFromPath("npm"), File.Exists(Path.Combine(workingDirectory, "package-lock.json")) ? "ci" : "install", workingDirectory).Wait();
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable workingDirectory is used on lines 278, 280, and 282 but is never defined in the ProcessRecord method. This will cause a compilation error. Based on the context, it appears this should be Path.GetDirectoryName(tempTSPLocation) which is used for the subsequent tsp compile command.

Copilot uses AI. Check for mistakes.
{
Scheme = "https",
Host = "raw.githubusercontent.com",
Path = $"{RemoteRepositoryName ?? "Azure/azure-rest-api-specs"}/{RemoteCommit ?? "main"}/{RemoteDirectory ?? ""}/tspconfig.yaml"
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 138 has redundant null-coalescing operators since the condition on line 132 already checks that these values are not null or empty. The fallback values "Azure/azure-rest-api-specs", "main", and "" will never be used. Remove the ?? operators for clarity.

Suggested change
Path = $"{RemoteRepositoryName ?? "Azure/azure-rest-api-specs"}/{RemoteCommit ?? "main"}/{RemoteDirectory ?? ""}/tspconfig.yaml"
Path = $"{RemoteRepositoryName}/{RemoteCommit}/{RemoteDirectory}/tspconfig.yaml"

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 18, 2025 04:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment on lines +60 to +63
/// 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$";
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comment about GitHub SHA-256 hash migration is valuable context but creates potential technical debt. If GitHub does move to SHA-256 (64-character) commit hashes, this regex pattern will silently fail to match valid URLs. Consider either supporting both hash lengths now (40 or 64 characters) or adding validation that provides a clear error message when an unexpected hash length is encountered.

Copilot uses AI. Check for mistakes.
Comment on lines +588 to +711
Console.WriteLine($"Downloading TSP config from: {uri}");

// Prepare request and timeout
using var request = new HttpRequestMessage(HttpMethod.Get, uri);
request.Headers.Add("User-Agent", "AzDev-TSPConfig/1.0");

using var cts = new System.Threading.CancellationTokenSource(TimeSpan.FromMinutes(2));

try
{
// Send request and get response
using var response = await httpClient.SendAsync(request, cts.Token);

// Check response status
if (!response.IsSuccessStatusCode)
{
throw new HttpRequestException($"Failed to download TSP config. Status: {response.StatusCode}, Reason: {response.ReasonPhrase}");
}

// Read and validate content
var content = await response.Content.ReadAsStringAsync();

if (string.IsNullOrWhiteSpace(content))
{
throw new InvalidOperationException("Downloaded TSP config content is empty");
}

Console.WriteLine($"Successfully downloaded TSP config ({content.Length} characters)");
return content;
}
catch (TaskCanceledException ex) when (ex.InnerException is TimeoutException || ex.CancellationToken.IsCancellationRequested)
{
throw new TimeoutException($"Timeout occurred while downloading TSP config from {uri}", ex);
}
catch (HttpRequestException ex)
{
throw new InvalidOperationException($"Network error occurred while downloading TSP config: {ex.Message}", ex);
}
catch (Exception ex)
{
throw new Exception($"Failed to download tspconfig from {uri}, {ex.Message}", ex);
}
}

private async Task<string> GetTSPConfigLocal(string uri)
{
// Validate uri
if (string.IsNullOrWhiteSpace(uri))
{
throw new ArgumentException("URI cannot be null or empty", nameof(uri));
}

// Normalize and validate the path
string normalizedPath;
try
{
normalizedPath = Path.GetFullPath(uri);
}
catch (Exception ex)
{
throw new ArgumentException($"Invalid file path: {uri}", nameof(uri), ex);
}

// Check if file exists
if (!File.Exists(normalizedPath))
{
throw new FileNotFoundException($"TSP config file not found: {normalizedPath}", normalizedPath);
}

Console.WriteLine($"Reading TSP config from local file: {normalizedPath}");

try
{
// Read file content asynchronously
var content = await File.ReadAllTextAsync(normalizedPath);

if (string.IsNullOrWhiteSpace(content))
{
throw new InvalidOperationException($"TSP config file is empty: {normalizedPath}");
}

Console.WriteLine($"Successfully read TSP config from local file ({content.Length} characters)");
return content;
}
catch (UnauthorizedAccessException ex)
{
throw new UnauthorizedAccessException($"Access denied reading TSP config file: {normalizedPath}", ex);
}
catch (IOException ex)
{
throw new InvalidOperationException($"IO error reading TSP config file: {normalizedPath} - {ex.Message}", ex);
}
catch (Exception ex)
{
throw new Exception($"Failed to read tspconfig from {normalizedPath}, {ex.Message}", ex);
}
}

private object MergeTSPConfig(string parentConfigPath, string childConfigPath)
{
string parentConfig = GetTSPConfig(parentConfigPath);

// Validate and deserialize parent config
if (string.IsNullOrWhiteSpace(parentConfig) || !YamlHelper.TryDeserialize<IDictionary<object, object>>(parentConfig, out IDictionary<object, object> parent))
{
throw new ArgumentException("Invalid parent TSP config: " + parentConfig, nameof(parentConfig));
}

// return parent if no child config provided
if (string.IsNullOrEmpty(childConfigPath))
{
Console.WriteLine($"No ChildConfig provided, use {parentConfigPath}");
return parent;
}
string childConfig = GetTSPConfig(childConfigPath);
// Validate and deserialize child config
if (string.IsNullOrWhiteSpace(childConfig) || !YamlHelper.TryDeserialize<IDictionary<object, object>>(childConfig, out IDictionary<object, object> child))
{
throw new ArgumentException("Invalid child TSP config: " + childConfig, nameof(childConfig));
}

Console.WriteLine("Performing deep merge for parent: " + parentConfigPath + " and child: " + childConfigPath);
var mergedConfig = MergeNestedObjectIteratively(parent, child);
Console.WriteLine("TSP config merge completed successfully");
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code writes informational messages using Console.WriteLine instead of PowerShell cmdlet methods. Replace Console.WriteLine with WriteVerbose for informational messages so users can control output verbosity using the -Verbose flag. This provides better PowerShell integration and user experience.

Copilot uses AI. Check for mistakes.
Comment on lines 278 to 283
if (!File.Exists(Path.Combine(tempTSPLocation, "package.json")))
{
throw new FileNotFoundException($"package.json not found in {tempTSPLocation}");
}
RunCommand(FindNPMCommandFromPath("npm"), File.Exists(Path.Combine(tempTSPLocation, "package-lock.json")) ? "ci" : "install", tempTSPLocation).Wait();
RunCommand(FindNPMCommandFromPath("tsp"), $"compile ./ --emit {EmitterPath ?? emitterName} --output-dir {emitterOutputDir}", Path.GetDirectoryName(tempTSPLocation)).Wait();
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path checking logic attempts to validate if a directory exists on line 278 but the variable tempTSPLocation represents a file path (specifically tspconfig.yaml), not a directory. This File.Exists check on line 249 should validate a file, but the error message on line 251 refers to it as a "TSP config file" which is correct. However, the check on line 278 uses File.Exists on tempTSPLocation expecting a package.json, but tempTSPLocation is the tspconfig.yaml file path. Line 278 should construct the path to package.json by using Path.GetDirectoryName(tempTSPLocation) first.

Suggested change
if (!File.Exists(Path.Combine(tempTSPLocation, "package.json")))
{
throw new FileNotFoundException($"package.json not found in {tempTSPLocation}");
}
RunCommand(FindNPMCommandFromPath("npm"), File.Exists(Path.Combine(tempTSPLocation, "package-lock.json")) ? "ci" : "install", tempTSPLocation).Wait();
RunCommand(FindNPMCommandFromPath("tsp"), $"compile ./ --emit {EmitterPath ?? emitterName} --output-dir {emitterOutputDir}", Path.GetDirectoryName(tempTSPLocation)).Wait();
string tempTspDirectory = Path.GetDirectoryName(tempTSPLocation);
if (string.IsNullOrEmpty(tempTspDirectory))
{
throw new InvalidOperationException($"Invalid TSP config path: {tempTSPLocation}");
}
if (!File.Exists(Path.Combine(tempTspDirectory, "package.json")))
{
throw new FileNotFoundException($"package.json not found in {tempTspDirectory}");
}
RunCommand(
FindNPMCommandFromPath("npm"),
File.Exists(Path.Combine(tempTspDirectory, "package-lock.json")) ? "ci" : "install",
tempTspDirectory).Wait();
RunCommand(
FindNPMCommandFromPath("tsp"),
$"compile ./ --emit {EmitterPath ?? emitterName} --output-dir {emitterOutputDir}",
tempTspDirectory).Wait();

Copilot uses AI. Check for mistakes.
Comment on lines 309 to 323
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Console.WriteLine($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine($"##########DEBUG: looking for path: {pathEnv} ##########");
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
Console.WriteLine($"##########DEBUG: looking for npm path: {_npmPath} ##########");
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
Console.WriteLine($"##########DEBUG: Found npm command path: {commandPath} ##########");
if (!File.Exists(commandPath))
{

Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for detecting npm path searches for any PATH entry ending with "npm", which could incorrectly match directories like "/usr/local/npm" or "/opt/npm" that aren't the actual npm binary directory. This could cause the command resolution to fail. Consider using a more robust approach like checking for the existence of npm executable in each PATH directory, or using "where npm" (Windows) or "which npm" (Unix) commands.

Suggested change
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":"";
Console.WriteLine($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine($"##########DEBUG: looking for path: {pathEnv} ##########");
string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
Console.WriteLine($"##########DEBUG: looking for npm path: {_npmPath} ##########");
string commandPath = Path.Combine(_npmPath, command+commandSuffix);
Console.WriteLine($"##########DEBUG: Found npm command path: {commandPath} ##########");
if (!File.Exists(commandPath))
{
string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd" : "";
Console.WriteLine($"##########DEBUG: looking for suffix: {commandSuffix} ##########");
// If we do not yet have a cached path, or the cached path no longer contains the command, search PATH.
string cachedCommandPath = string.IsNullOrEmpty(_npmPath)
? null
: Path.Combine(_npmPath, command + commandSuffix);
if (string.IsNullOrEmpty(_npmPath) || !File.Exists(cachedCommandPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
Console.WriteLine($"##########DEBUG: searching PATH for command: {pathEnv} ##########");
_npmPath = null;
foreach (var pathEntry in pathEnv.Split(Path.PathSeparator))
{
if (string.IsNullOrWhiteSpace(pathEntry))
{
continue;
}
string trimmedEntry = pathEntry.Trim();
string candidatePath = Path.Combine(trimmedEntry, command + commandSuffix);
Console.WriteLine($"##########DEBUG: checking candidate command path: {candidatePath} ##########");
if (File.Exists(candidatePath))
{
_npmPath = trimmedEntry;
cachedCommandPath = candidatePath;
Console.WriteLine($"##########DEBUG: found npm command directory: {_npmPath} ##########");
break;
}
}
if (string.IsNullOrEmpty(_npmPath) || string.IsNullOrEmpty(cachedCommandPath) || !File.Exists(cachedCommandPath))
{
throw new FileNotFoundException($"Command '{command}' not found in system PATH.");
}
}
Console.WriteLine($"##########DEBUG: using npm path: {_npmPath} ##########");
string commandPath = cachedCommandPath ?? Path.Combine(_npmPath, command + commandSuffix);
Console.WriteLine($"##########DEBUG: Found npm command path: {commandPath} ##########");
if (!File.Exists(commandPath))
{

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 19, 2025 07:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment on lines 543 to 552
//if tspconfig emitted previously was from local, only record the absolute directory name
if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"]))
{
if (remoteInfo != (null, null, null, null))
{
throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`.");
}
return (string)tspLocationPWDContent["directory"];
}
(string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo;
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tuple comparison to (null, null, null, null) may not work as expected. Tuple equality compares element-by-element, but the individual elements are strings, not null literals. This condition will never be true if remoteInfo contains actual null values. Consider checking each element individually or using a different comparison pattern.

Suggested change
//if tspconfig emitted previously was from local, only record the absolute directory name
if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"]))
{
if (remoteInfo != (null, null, null, null))
{
throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`.");
}
return (string)tspLocationPWDContent["directory"];
}
(string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo;
(string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo;
//if tspconfig emitted previously was from local, only record the absolute directory name
if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"]))
{
bool hasRemoteInfo =
!string.IsNullOrEmpty(RemoteDirectory) ||
!string.IsNullOrEmpty(RemoteCommit) ||
!string.IsNullOrEmpty(RemoteRepositoryName) ||
!string.IsNullOrEmpty(RemoteForkName);
if (hasRemoteInfo)
{
throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`.");
}
return (string)tspLocationPWDContent["directory"];
}

Copilot uses AI. Check for mistakes.
## Notes

- The cmdlet relies on `npm` and the `@azure-tools/typespec-powershell` emitter.
- It handles the download and setup of necessary TypeSpec files in a temporary directory (unless `SkipCleanTemp` is used).
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The abbreviation "TSP" is used extensively throughout the document but is only defined once at the beginning (line 7). While this is acceptable, the term "TypeSpec" should be used consistently or "TSP" should be defined on first use in each major section for clarity, especially since this is user-facing documentation.

Suggested change
- It handles the download and setup of necessary TypeSpec files in a temporary directory (unless `SkipCleanTemp` is used).
- It handles the download and setup of necessary TypeSpec (TSP) files in a temporary directory (unless `SkipCleanTemp` is used).

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +13
- **TSP Location**: The source of truth for the TypeSpec configuration. It can be a remote URL (e.g., GitHub) or a local file path. There must be ***tspconfig.yaml*** and ***typespec files*** under this location.
- ***remote***
- ***local***
- ***tsp-location.yaml***: Will be calculated from `./tsp-location.yaml` if not provided and tsp-location.yaml exists. See [`tsp-location.yaml`](#tsp-location-yaml)
- **AzPSConfig**: An optional, additional TypeSpec configuration file (typically `tspconfig.yaml` in the local directory) that is merged with the main TSP configuration. This allows for Azure PowerShell-specific overrides or extensions. Will look for `./tspconfig.yaml` if not provided.
- <a id="tsp-location-yaml"></a>**tsp-location.yaml**: A local file generated by this cmdlet to persist the source location of the TypeSpec configuration. This allows subsequent runs to resolve the configuration without needing to re-specify the location parameters.
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent terminology: Line 11 uses "tsp-location.yaml" while line 13 references the same concept with different formatting. The terminology section should use consistent formatting (either code blocks or bold) for technical terms throughout.

Suggested change
- **TSP Location**: The source of truth for the TypeSpec configuration. It can be a remote URL (e.g., GitHub) or a local file path. There must be ***tspconfig.yaml*** and ***typespec files*** under this location.
- ***remote***
- ***local***
- ***tsp-location.yaml***: Will be calculated from `./tsp-location.yaml` if not provided and tsp-location.yaml exists. See [`tsp-location.yaml`](#tsp-location-yaml)
- **AzPSConfig**: An optional, additional TypeSpec configuration file (typically `tspconfig.yaml` in the local directory) that is merged with the main TSP configuration. This allows for Azure PowerShell-specific overrides or extensions. Will look for `./tspconfig.yaml` if not provided.
- <a id="tsp-location-yaml"></a>**tsp-location.yaml**: A local file generated by this cmdlet to persist the source location of the TypeSpec configuration. This allows subsequent runs to resolve the configuration without needing to re-specify the location parameters.
- **TSP Location**: The source of truth for the TypeSpec configuration. It can be a remote URL (e.g., GitHub) or a local file path. There must be `tspconfig.yaml` and TypeSpec files under this location.
- ***remote***
- ***local***
- `tsp-location.yaml`: Will be calculated from `./tsp-location.yaml` if not provided and `tsp-location.yaml` exists. See [`tsp-location.yaml`](#tsp-location-yaml)
- **AzPSConfig**: An optional, additional TypeSpec configuration file (typically `tspconfig.yaml` in the local directory) that is merged with the main TSP configuration. This allows for Azure PowerShell-specific overrides or extensions. Will look for `./tspconfig.yaml` if not provided.
- <a id="tsp-location-yaml"></a>`tsp-location.yaml`: A local file generated by this cmdlet to persist the source location of the TypeSpec configuration. This allows subsequent runs to resolve the configuration without needing to re-specify the location parameters.

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +30
## Prerequisite
- **node version >= 20**
- **typespec compiler installed?**: `npm install -g @typespec/compiler`
- **git clone/pull**
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing article: "typespec compiler installed?" should be "Is the TypeSpec compiler installed?" for proper grammar. The prerequisite section should use complete sentences for clarity.

Suggested change
## Prerequisite
- **node version >= 20**
- **typespec compiler installed?**: `npm install -g @typespec/compiler`
- **git clone/pull**
## Prerequisites
- Node.js version 20 or later is installed.
- The TypeSpec compiler is installed. You can install it with: `npm install -g @typespec/compiler`
- The azure-powershell repository has been cloned or updated with the latest changes.

Copilot uses AI. Check for mistakes.
Comment on lines 116 to 119
displayName: Install npm globally
inputs:
command: custom
customCommand: install -g npm
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation of this task definition is incorrect. Lines 116-119 should be indented to align with the sibling tasks above (lines 115 should be at the same level as line 112). The displayName on line 116 should be indented with two spaces like line 113.

Suggested change
displayName: Install npm globally
inputs:
command: custom
customCommand: install -g npm
displayName: Install npm globally
inputs:
command: custom
customCommand: install -g npm

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 19, 2025 08:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 13 comments.

throw new InvalidOperationException($"TSP config file is empty: {normalizedPath}");
}

Console.WriteLine($"Successfully read TSP config from local file ({content.Length} characters)");
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 'Console.WriteLine' directly in a PowerShell cmdlet bypasses PowerShell's output stream system. Use WriteVerbose or WriteInformation instead to properly integrate with PowerShell's stream handling.

Copilot uses AI. Check for mistakes.
Comment on lines +707 to +719
Console.WriteLine($"No ChildConfig provided, use {parentConfigPath}");
return parent;
}
string childConfig = GetTSPConfig(childConfigPath);
// Validate and deserialize child config
if (string.IsNullOrWhiteSpace(childConfig) || !YamlHelper.TryDeserialize<IDictionary<object, object>>(childConfig, out IDictionary<object, object> child))
{
throw new ArgumentException("Invalid child TSP config: " + childConfig, nameof(childConfig));
}

Console.WriteLine("Performing deep merge for parent: " + parentConfigPath + " and child: " + childConfigPath);
var mergedConfig = MergeNestedObjectIteratively(parent, child);
Console.WriteLine("TSP config merge completed successfully");
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 'Console.WriteLine' directly in a PowerShell cmdlet bypasses PowerShell's output stream system. Use WriteVerbose or WriteInformation instead to properly integrate with PowerShell's stream handling.

Copilot uses AI. Check for mistakes.
Comment on lines +337 to +338
private string NormalizePath(string path) => path.Replace('/', Path.DirectorySeparatorChar).Replace('\\', Path.DirectorySeparatorChar);

Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NormalizePath method at line 337 will always produce the same result since both replacements use Path.DirectorySeparatorChar. This appears to be an attempt to normalize paths but doesn't handle the cross-platform case correctly. Instead, use Path.GetFullPath or ensure different separators are used for the replacement (e.g., replace both '/' and '\' with the platform-specific separator).

Suggested change
private string NormalizePath(string path) => path.Replace('/', Path.DirectorySeparatorChar).Replace('\\', Path.DirectorySeparatorChar);
private string NormalizePath(string path)
{
if (path == null)
{
return null;
}
// Normalize non-platform separators to the current platform's directory separator
if (Path.DirectorySeparatorChar == '/')
{
// Unix-like platforms: convert backslashes to forward slashes
return path.Replace('\\', Path.DirectorySeparatorChar);
}
// Windows and similar platforms: convert forward slashes to backslashes
return path.Replace('/', Path.DirectorySeparatorChar);
}

Copilot uses AI. Check for mistakes.
Comment on lines +327 to +329
catch
{
// Ignore invalid paths in PATH
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty catch block at lines 327-330 silently ignores all errors when checking for command existence in PATH. While the comment says "Ignore invalid paths in PATH", this could also hide other legitimate errors. Consider catching more specific exceptions or logging the suppressed errors for debugging purposes.

Suggested change
catch
{
// Ignore invalid paths in PATH
catch (Exception ex)
{
// Ignore invalid paths in PATH but log details for debugging
Debug.WriteLine($"Failed to check path '{path}' for command '{command}': {ex.Message}");

Copilot uses AI. Check for mistakes.
Comment on lines +466 to +471
Console.WriteLine(e.Data);
};
process.ErrorDataReceived += (sender, e) =>
{
if (!string.IsNullOrEmpty(e.Data))
Console.Error.WriteLine(e.Data);
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 'Console.WriteLine' and 'Console.Error.WriteLine' directly is not idiomatic for PowerShell cmdlets. Use the WriteVerbose, WriteWarning, or WriteInformation methods provided by the PowerShell Cmdlet base class to provide proper output that respects PowerShell's stream handling and user preferences.

Suggested change
Console.WriteLine(e.Data);
};
process.ErrorDataReceived += (sender, e) =>
{
if (!string.IsNullOrEmpty(e.Data))
Console.Error.WriteLine(e.Data);
WriteVerbose(e.Data);
};
process.ErrorDataReceived += (sender, e) =>
{
if (!string.IsNullOrEmpty(e.Data))
WriteWarning(e.Data);

Copilot uses AI. Check for mistakes.
Comment on lines 550 to 563
//if tspconfig emitted previously was from local, only record the absolute directory name
if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"]))
{
if (remoteInfo != (null, null, null, null))
{
throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`.");
}
return (string)tspLocationPWDContent["directory"];
}
(string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo;
//otherwise it was from remote, construct its url
string repo = !string.IsNullOrEmpty(RemoteForkName) ? $"{RemoteForkName}/azure-rest-api-specs" : (!string.IsNullOrEmpty(RemoteRepositoryName) ? RemoteRepositoryName : (string)tspLocationPWDContent["repo"]);
string commit = !string.IsNullOrEmpty(RemoteCommit) ? RemoteCommit : (string)tspLocationPWDContent["commit"];
string directory = !string.IsNullOrEmpty(RemoteDirectory) ? RemoteDirectory : (string)tspLocationPWDContent["directory"];
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 551 contains a potential null reference issue. The code casts 'tspLocationPWDContent["directory"]' and other dictionary values to string without null checking first. If these values are null or missing from the dictionary, this will throw an exception. Use safe casting with 'as string' or check ContainsKey before accessing.

Suggested change
//if tspconfig emitted previously was from local, only record the absolute directory name
if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"]))
{
if (remoteInfo != (null, null, null, null))
{
throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`.");
}
return (string)tspLocationPWDContent["directory"];
}
(string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo;
//otherwise it was from remote, construct its url
string repo = !string.IsNullOrEmpty(RemoteForkName) ? $"{RemoteForkName}/azure-rest-api-specs" : (!string.IsNullOrEmpty(RemoteRepositoryName) ? RemoteRepositoryName : (string)tspLocationPWDContent["repo"]);
string commit = !string.IsNullOrEmpty(RemoteCommit) ? RemoteCommit : (string)tspLocationPWDContent["commit"];
string directory = !string.IsNullOrEmpty(RemoteDirectory) ? RemoteDirectory : (string)tspLocationPWDContent["directory"];
tspLocationPWDContent.TryGetValue("directory", out object directoryObj);
tspLocationPWDContent.TryGetValue("repo", out object repoObj);
tspLocationPWDContent.TryGetValue("commit", out object commitObj);
string existingDirectory = directoryObj as string;
string existingRepo = repoObj as string;
string existingCommit = commitObj as string;
//if tspconfig emitted previously was from local, only record the absolute directory name
if (!string.IsNullOrEmpty(existingDirectory) &&
File.Exists(existingDirectory) &&
string.IsNullOrEmpty(existingRepo) &&
string.IsNullOrEmpty(existingCommit))
{
if (remoteInfo != (null, null, null, null))
{
throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`.");
}
return existingDirectory;
}
(string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo;
//otherwise it was from remote, construct its url
string repo = !string.IsNullOrEmpty(RemoteForkName)
? $"{RemoteForkName}/azure-rest-api-specs"
: (!string.IsNullOrEmpty(RemoteRepositoryName)
? RemoteRepositoryName
: existingRepo);
string commit = !string.IsNullOrEmpty(RemoteCommit) ? RemoteCommit : existingCommit;
string directory = !string.IsNullOrEmpty(RemoteDirectory) ? RemoteDirectory : existingDirectory;
if (string.IsNullOrEmpty(repo) || string.IsNullOrEmpty(commit) || string.IsNullOrEmpty(directory))
{
throw new ArgumentException("tspLocation metadata is missing required fields: repo, commit, or directory.");
}

Copilot uses AI. Check for mistakes.
throw new FileNotFoundException($"TSP config file not found: {normalizedPath}", normalizedPath);
}

Console.WriteLine($"Reading TSP config from local file: {normalizedPath}");
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 'Console.WriteLine' directly in a PowerShell cmdlet bypasses PowerShell's output stream system. Use WriteVerbose or WriteInformation instead to properly integrate with PowerShell's stream handling.

Copilot uses AI. Check for mistakes.
//if tspconfig emitted previously was from local, only record the absolute directory name
if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"]))
{
if (remoteInfo != (null, null, null, null))
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tuple comparison at line 553 uses a pattern that may not work as intended. In C#, tuple equality checks each element, so this condition will only be true if all four elements are null. If the intention is to check if any remote info is provided, consider using a more explicit condition like checking if all elements are null or empty.

Suggested change
if (remoteInfo != (null, null, null, null))
if (remoteInfo.Item1 != null || remoteInfo.Item2 != null || remoteInfo.Item3 != null || remoteInfo.Item4 != null)

Copilot uses AI. Check for mistakes.
Comment on lines 76 to 77
private string _npmPath = "";

Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name '_npmPath' is declared but never used in the class. This should be removed to avoid confusion and maintain clean code.

Suggested change
private string _npmPath = "";

Copilot uses AI. Check for mistakes.
throw new InvalidOperationException("Downloaded TSP config content is empty");
}

Console.WriteLine($"Successfully downloaded TSP config ({content.Length} characters)");
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 'Console.WriteLine' directly in a PowerShell cmdlet bypasses PowerShell's output stream system. Use WriteVerbose or WriteInformation instead to properly integrate with PowerShell's stream handling.

Suggested change
Console.WriteLine($"Successfully downloaded TSP config ({content.Length} characters)");
WriteVerbose($"Successfully downloaded TSP config ({content.Length} characters)");

Copilot uses AI. Check for mistakes.
@VeryEarly VeryEarly closed this Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant