fix: fcli tool definitions update should validate definitions zip file

[jmadhur87]

No description provided.

[rsenden]

Please update your IDE settings to use 4 spaces instead of tabs for indentation, then fix all files in your PR to use spaces instead of tabs.

[rsenden]

Again, method is way too long, making ti difficult to understand exact behavior. Why is there a difference in processing depending on whether isUpdateDefault returns true or false, and why is isUpdateDefault named like that; it just checks whether toolDefinitionCustomFilePath contains https://; what has that to do with default?

[jmadhur87]

Instead of isUpdateDefault, directly checking the condition now.
Its verifying if this update command does not contain any source

[rsenden]

Using a static mutable field like toolDefinitionCustomFilePath is generally considered bad practice. It introduces shared state across all usages of the class, which can lead to race conditions, unexpected side effects, and makes the code harder to test and maintain. Prefer passing such values as method parameters or encapsulating them in an instance field within a properly scoped object.

[rsenden]

@jmadhur87, can you please review the GitHub Copilot feedback to see whether any of this would be useful to implement/fix?

---

Useful changes overall — the new behavior (user ZIP support, update modes, "actionResult" reporting) looks reasonable. A few potential bugs, robustness issues and style/maintenance improvements to consider:

Bugs / correctness issues

extractYamlFromZip compares ZipEntry.getName() directly to yamlFile

Problem: entries often have a path (dir/name.yaml). You should compare the file name only (Path.of(entry.getName()).getFileName().toString()) like you do elsewhere. Otherwise the YAML might not be extracted even when present.
Fix: change the equality check to compare filename-only.
NPE when using ZipEntry.getLastModifiedTime()

Several places assume entry.getLastModifiedTime() != null (e.g. addYamlDescriptor, addYamlOutputDescriptors when creating new Date(entry.getLastModifiedTime().toMillis())) — that will NPE if the entry doesn't have a timestamp.
Fix: null-check the LastModifiedTime and use null or a fallback time when it is missing.
getModifiedTime may return null, then new Date(getModifiedTime(dest).toMillis()) will NPE

getModifiedTime now returns null if file doesn't exist. update(...) calls getModifiedTime(dest).toMillis() without checking null. In practice dest should exist, but defensively check for null before using toMillis().
Fix: either ensure getModifiedTime never returns null (throw if missing) or handle the null case in update().
isValidZip only checks whether any required YAML file exists in the zip

You accept zips that contain at least one required yaml. That may allow partial/missing tool definitions. You do later fill missing files from other sources, but make sure the intended behavior is "accept if zip has at least one required yaml" (likely intended) and document it.
Suggestion: if the intention is to accept only fully-contained zips, require all required YAML files.
Path traversal / name sanitization

You usually use Path.of(entry.getName()).getFileName() which mitigates path traversal, but ensure every place writing files/entries uses only the filename (not the full entry name). In mergeDefinitionsZip you write each found file into the output zip with ZipEntry(file.getName()) — good.
MalformedURLException catch logic

You attempt UnirestHelper.download(... new URL(source).toString()) and on MalformedURLException treat source as a local file. Consider other URL issues (e.g., unreachable, 404) that will not throw MalformedURLException but will fail in UnirestHelper.download; ensure failures are handled and have clear error messages.
Exception handling: losing original cause

In several places you throw new FcliSimpleException("Error Loading User ZIP file: " + zipPath) or similar but don't include the original exception as the cause. Include the caught exception so callers can see the stacktrace:
throw new FcliSimpleException("Error Loading User ZIP file: " + zipPath, e);
Regex parsing for duration allows multiple tokens but rejects seconds, which is fine; however the error messages are a bit generic. Consider validating earlier (on CLI) to provide better feedback.

Robustness / resource handling

Good: try-with-resources used in most places. Ensure all streams are closed (e.g., in mergeDefinitionsZip you open InputStream in try-with-resources, fine).
Consider adding more logging (info/debug) for key actions (download, using user ZIP vs state vs internal) to help debugging.
API / design / style suggestions

ToolRegistry list maintenance / dynamic registration
You already leave TODO; consider ServiceLoader or a platform-specific registration mechanism so tools can register themselves and avoid drift between code and registry.
ArgGroup handling in ToolDefinitionsUpdateCommand
You set exclusive = true so picocli should ensure mutual exclusion; explicit runtime check is redundant. It's OK to keep a guard but it's duplicative.
UpdateMode paramLabel strings
paramLabel = "Force Update" is not a typical param placeholder (like ); consider removing paramLabel or use a short, meaningful placeholder.
Use clearer error strings and include the failing path or URL and the cause for easier debugging.
Unit tests
Add unit tests for:
parseDurationToDate edge cases and invalid formats
isValidZip with zips that have no matching files / partial matches
mergeDefinitionsZip behavior when some YAMLs are present in user zip and others are taken from internal zip
extractYamlFromZip with entries containing path components
Performance / minor issues

Some methods are long and do multiple responsibilities (mergeDefinitionsZip, addYamlOutputDescriptors). Consider splitting into smaller methods (you already did some splitting — good).
Minor: format and indentation in the diff appears inconsistent; run code formatter / checkstyle.
Security considerations

ZIP handling: always be careful with zip entries that contain large sizes or compressed bombs. You don't seem to unpack arbitrary nested directories (you copy entries directly), but consider bounds checks if working with untrusted zips. Also you currently extract to DEFINITIONS_STATE_DIR; ensure you validate/limit size and number of extracted files.
Small correctness nitpicks

getRequiredYamlFileNames uses ToolRegistry.getRegisteredToolNames().stream().map(s -> s + ".yaml").collect(Collectors.toSet()); fine but consider making ToolRegistry provide the list already suffixed or document expected format.
ToolDefinitionsOutputDescriptor: adding actionResult is good. Ensure consumers of this DTO (JSON serializers / CLI output) handle the added property (you did annotate with JsonProperty using IActionCommandResultSupplier.actionFieldName — good).
Suggested concrete code fixes (high-priority)

In extractYamlFromZip:
change: if (entry.getName().equals(yamlFile)) {
to: if (Path.of(entry.getName()).getFileName().toString().equals(yamlFile)) {
In places that build new Date(entry.getLastModifiedTime().toMillis()), guard:
var lastModified = entry.getLastModifiedTime() != null ? new Date(entry.getLastModifiedTime().toMillis()) : null;
In update(), check getModifiedTime(dest) != null before calling toMillis() (or throw a descriptive exception if it unexpectedly missing).
Include original exceptions when wrapping in FcliSimpleException.
If you want, I can:

Produce a focused patch with the fixes above (extractYamlFromZip filename check, null-checks for lastModifiedTime, include causes in exceptions, fix getModifiedTime usage).
Run a quick search for other places that assume non-null last-modified and produce a complete list to change.
Which of the concrete fixes would you like me to prepare as a patch?