Use valid CLI version probes - #4027
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughCLI scrapers now pass tool-specific version arguments to availability probes. Executor implementations support argument-aware checks and fallback behavior. Liquibase parsing now extracts versions from multiple banner formats. ChangesCLI version detection
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR updates CLI availability checks to use supported version probes and adds focused regression coverage; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CliScraperBase
participant ICliCommandExecutor
participant CLI
CliScraperBase->>ICliCommandExecutor: IsAvailableAsync(command, VersionArguments)
ICliCommandExecutor->>CLI: Execute version arguments
CLI-->>ICliCommandExecutor: Return exit status and output
ICliCommandExecutor-->>CliScraperBase: Return availability result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR updates CLI availability checks to use each scraper’s supported version arguments and adds focused regression coverage.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliScraperBase.cs | Availability checks now pass each scraper’s version arguments into the executor. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/TypeDetection/ProcessCliCommandExecutor.cs | Adds argument-aware probing while retaining the existing help fallback. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/TypeDetection/ResilientCliCommandExecutor.cs | Forwards argument-aware availability checks directly to the inner executor. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/LiquibaseCliScraper.cs | Extracts a stable semantic version from supported Liquibase banner formats. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/CliVersionProbeTests.cs | Verifies that affected scrapers use their supported version subcommands. |
Sequence Diagram
sequenceDiagram
participant S as CLI Scraper
participant R as Resilient Executor
participant P as Process Executor
participant C as CLI Binary
S->>R: IsAvailableAsync(executable, VersionArguments)
R->>P: Forward tool-specific probe
P->>C: Execute VersionArguments
alt probe succeeds
C-->>P: Exit code 0
P-->>S: Available
else probe fails
P->>C: Execute --help
C-->>P: Help result
P-->>S: Availability result
end
Reviews (5): Last reviewed commit: "test(generator): cover real version fall..." | Re-trigger Greptile
GitHub Actions
Inactive
Code reviewSummary: This PR fixes Architectural concern: availability-check fallback silently removed for every other scraper
and That replaces the old 2-arg
Why this matters: the fix for 6 specific tools was implemented by changing a shared code path that every scraper flows through, and the resilience behavior that used to protect the other 45 was dropped as a side effect rather than a deliberate choice. Nothing in the diff or tests exercises "tool installed, Suggested approach: preserve the old fallback behavior for scrapers that haven't opted into a tool-specific probe. For example, make No CLAUDE.md compliance issues — there's no nested |
|
@claude Please re-review exact head \ |
|
@claude Please re-review exact head 0d52298. The argument-aware overload now preserves the legacy tolerant --help fallback, the two-argument overload delegates to it, and a cross-platform regression covers --version failure followed by successful --help. Validation: ProcessCliCommandExecutorTests 18/18, CliVersionProbeTests 5/5, ResilientCliCommandExecutorTests 1/1. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review (re-review at 0d52298)
Previous architectural concern is resolved. The earlier review flagged that routing CliScraperBase.IsAvailableAsync() through the new 3-arg IsAvailableAsync(command, arguments, ct) dropped the --help fallback for the ~45 scrapers that don't override VersionArguments. That's fixed: ProcessCliCommandExecutor.IsAvailableAsync(command, arguments, ct) now falls back to --help when the initial probe fails (ProcessCliCommandExecutor.cs:508-516) and treats any exit code other than -1 as "available," matching the old 2-arg overload's tolerance. ResilientCliCommandExecutorTests and the new Argument_Aware_IsAvailableAsync_Falls_Back_To_Help test cover this path. Good fix, and thanks for pointing me at the exact head to re-check.
Two smaller things worth a look before merge:
1. HelmCliScraper's strict availability check is now gone
HelmCliScraper.cs previously overrode IsAvailableAsync to require Executor.ExecuteAsync(ExecutablePath, VersionArguments, ct).Success directly — i.e., helm version itself had to succeed. That override is deleted in this PR, so Helm now falls through to CliScraperBase's default, which uses the new tolerant IsAvailableAsync(command, arguments, ct) (probe version, fall back to --help, accept any exit code short of -1).
Net effect: a Helm install where helm version fails (corrupted plugin index, broken config, non-zero exit) but helm --help still exits 0 now reports available, whereas before it correctly reported unavailable. ScrapeAsync will then proceed into full command discovery for a tool whose version probe is broken, with GetVersionAsync() silently returning null.
This may well be intentional cleanup — the old strict override combined with the base class's own probing was actually causing Helm's executor to be invoked twice with "version" (see the HelmCliScraperTests diff, which changes the expected call list from ["version", "version"] to a single ["version"]), so removing the duplicate-triggering override is reasonable. But the side effect is a real loosening of Helm's specific availability semantics that isn't mentioned in the PR description. Worth either a one-line callout that this is deliberate (Helm now matches everyone else's tolerance) or, if the stricter check was actually load-bearing for Helm, keeping a narrower override that still uses the shared fallback machinery but requires the primary probe (not just --help) to succeed.
2. The new ICliCommandExecutor.IsAvailableAsync(command, arguments, ct) default-interface-method silently degrades unaware implementers
Task<bool> IsAvailableAsync(
string command,
string arguments,
CancellationToken cancellationToken = default) =>
IsAvailableAsync(command, cancellationToken);Both production implementers (ProcessCliCommandExecutor, ResilientCliCommandExecutor) correctly override this, so today's behavior is fine. But because the default silently ignores arguments and delegates to the old single-probe overload, any future ICliCommandExecutor implementation (a decorator, a mock, a test double) that forgets to override the 3-arg version will silently drop every scraper's tool-specific VersionArguments with no compiler warning — quietly defeating the whole point of this PR for that implementer. Since this interface only has two implementers in-tree, consider making the 3-arg member abstract (no default body) so the compiler forces every implementer to make an explicit choice, rather than leaning on a default that's easy to forget to override correctly.
Neither of these blocks the fix for the six tools this PR targets (ArgoCD, eksctl, Cosign, Go, Kubectl, Kustomize) — the new CliVersionProbeTests and KubectlCliScraperTests correctly verify those probes. They're worth a decision (intentional vs. oversight) before merge given the fallback-loosening theme is exactly what the previous round of review was about.
|
@claude Please review exact head c62c59f. It preserves the legacy availability fallback and now extracts machine-readable Liquibase versions after the CLI banner. Validation: ProcessCliCommandExecutorTests 18/18, CliVersionProbeTests 5/5, ResilientCliCommandExecutorTests 1/1, LiquibaseCliScraperTests 13/13. |
There was a problem hiding this comment.
Code review (re-review at c62c59f5)
Re-checked against the specific request: this head adds LiquibaseCliScraper.ParseVersionOutput (extracting a machine-readable version from the CLI banner) on top of the availability-fallback fix from the previous round. Validation numbers check out against the diff (LiquibaseCliScraperTests cover both the "Liquibase Version:" and "Liquibase Community X.X.X by Liquibase" banner shapes).
1. Previous Helm finding is still unaddressed
The prior round flagged that HelmCliScraper's custom IsAvailableAsync override (which required helm version itself to succeed) was deleted, and asked for a decision: was this an intentional loosening of Helm's availability semantics, or an oversight from deduplicating the double "version" invocation? At c62c59f5, HelmCliScraper.cs still has no IsAvailableAsync override — it falls through to the shared tolerant path (probe version, fall back to --help, accept anything but ExitCode == -1). That's not wrong, but since it was called out for a decision and the PR description/commits since then don't mention it, it's worth an explicit "yes, intentional" before merge rather than leaving it as a silent side effect of the dedup fix.
2. LiquibaseVersionPattern only recognizes the Community edition banner as a standalone match
[GeneratedRegex(
@"(?:Liquibase Version:|Liquibase (?:Community|'community' version))\s*(?<version>\d+(?:\.\d+)+(?:[-+][0-9A-Za-z.-]+)?)",
RegexOptions.IgnoreCase)]
private static partial Regex LiquibaseVersionPattern();The banner-only alternative hardcodes Community/'community' version. SupplementalGlobalOptions earlier in this same file references "Liquibase Secure" as an edition, so the scraper is already aware paid editions exist (e.g. Liquibase Pro X.X.X by Liquibase, Liquibase Secure X.X.X by Liquibase). In practice this is low risk — real Liquibase output pairs the banner with a separate Liquibase Version: X.X.X line, which the first alternative already matches — but if a probe ever only surfaces the edition banner without that line, ParseVersionOutput returns null and logs a warning instead of extracting a version, which is a narrower result than before (the previous default ParseVersionOutput at least returned the raw combined output). If you want this fully robust rather than "works for the shapes we've observed," consider widening the second alternative to Liquibase\s+\S+ (any single edition word) instead of enumerating specific editions, so a future edition name doesn't silently fall through to the null/warning path again.
Neither of these blocks the Liquibase version-parsing fix this round targets — both are pre-existing/edge-case concerns, not regressions introduced by this commit. Worth a quick "yes, intentional" on Helm and an optional regex widening for Liquibase before merge.
|
Addressed at exact head
@claude Please review exact head |
|
@claude Please review exact head |
GitHub Actions
Inactive
1 participant
Part of #3996.
VersionArgumentsValidation:
Summary by CodeRabbit
Improvements
Tests