refactor: migrate deprecated RequiredSubcommand and GetGeminiAPITarget callsites to canonical forms#44717
Conversation
…t callsites
- pip.go: RequiredSubcommand: "install" → RequiredSubcommands: []string{"install"} (canonical slice form)
- pip.go: remove redundant RequiredSubcommand: "" from uvxExtractor (zero value)
- npm.go: remove redundant RequiredSubcommand: "" from extractNpxFromCommands (zero value)
- awf_config.go: remove deprecated GetGeminiAPITarget warning comparison block (lines 516-519)
The else-if fallback for gemini engine backward compat is retained per ADR-34693.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Migrates deprecated
|
|
Hey One thing that would strengthen this PR:
If you’d like a hand, you can assign this prompt to your coding agent:
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44717 does not have the 'implementation' label and has only 7 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This PR completes a small cleanup in pkg/workflow by migrating remaining deprecated PackageExtractor.RequiredSubcommand callsites to the canonical RequiredSubcommands form, and by removing an outdated Gemini-vs-Antigravity override warning now that GEMINI_API_BASE_URL is deprecated.
Changes:
- Update the pip extractor to use
RequiredSubcommands: []string{"install"}instead of the deprecatedRequiredSubcommand: "install". - Remove redundant
RequiredSubcommand: ""zero-value initializations from theuvxandnpxextractors. - Remove the warning block that compared
GetGeminiAPITargetvsGetAntigravityAPITargetwhen both were configured, while preserving Gemini fallback routing forengine: gemini.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/pip.go | Migrates the pip extractor to RequiredSubcommands and drops a no-op empty RequiredSubcommand for uvx. |
| pkg/workflow/npm.go | Drops the no-op empty RequiredSubcommand for npx, relying on zero values. |
| pkg/workflow/awf_config.go | Removes an obsolete Gemini-vs-Antigravity override warning while keeping the Gemini fallback target path intact. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
There was a problem hiding this comment.
The changes are correct and clean.
- pip.go:
RequiredSubcommand: "install"properly migrated toRequiredSubcommands: []string{"install"}. Semantically equivalent using the canonical field. - pip.go, npm.go: Removing empty
RequiredSubcommand: ""fields is correct — zero values are implicit in Go structs. - awf_config.go: The deprecation warning removal is safe. The
else iffallback preservingGetGeminiAPITargetis correct per the backward-compatibility requirement in the PR body (ADR-34693).
No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 14.8 AIC · ⌖ 4.31 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — changes are clean and behaviour-preserving; approving.
📋 Key Themes & Highlights
Key Themes
- Correct zero-value omission: Dropping
RequiredSubcommand: ""from thenpxanduvxextractors is idiomatic Go — zero values should not be explicitly set. ✅ - Safe canonical migration:
RequiredSubcommand: "install"→RequiredSubcommands: []string{"install"}for pip is the right direction given the priority logic ingetRequiredSubcommands(). ✅ - Targeted removal in
awf_config.go: The deprecated warning block is cleanly excised without disturbing the backward-compatelse iffallback, consistent with ADR-34693. ✅
One Follow-up Opportunity (non-blocking)
package_extraction.go doc comments still show many examples using the deprecated RequiredSubcommand field (lines ~25, 37, 47, 67, 77, 84, 117, 127, 134, 224, 234). These are outside this PR's scope but could mislead future contributors. A follow-up to update those examples to RequiredSubcommands would complete the migration.
Positive Highlights
- ✅ PR description is clear and traces the rationale thoroughly, including the intentional retention of the backward-compat fallback.
- ✅ Existing test suite covers the migrated paths — no behaviour change means no new tests required here.
- ✅ Minimal diff footprint (7 additions / 13 deletions) for the scope of change.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 19.9 AIC · ⌖ 4.43 AIC · ⊞ 6.6K
Comment /matt to run again
There was a problem hiding this comment.
Non-blocking observations
The three callsite migrations (RequiredSubcommand → RequiredSubcommands, empty-string removal) are semantically correct — getRequiredSubcommands() has proper fallback logic and behavior is preserved.
Findings
1. Deleted warning had a bug — fixed by deletion rather than correction (awf_config.go): The removed log block printed geminiTarget twice instead of showing both geminiTarget and antigravityTarget. The PR drops the diagnostic entirely instead of fixing the format string. If operators can still configure both env vars simultaneously, this silently suppresses a useful misconfiguration warning.
2. Test file not migrated (package_extraction_test.go): Tests for pip still use the deprecated RequiredSubcommand field, meaning the new RequiredSubcommands code path is not exercised. Low risk since the fallback logic is trivial, but a complete migration should update the tests too.
🔎 Code quality review by PR Code Quality Reviewer · 39.2 AIC · ⌖ 4.61 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/awf_config.go:13
Removed warning had a latent bug — fix silently drops the diagnostic instead of correcting it.
<details>
<summary>💡 Details</summary>
The deleted block at original lines 516–519 had both %s format args pointing to geminiTarget, so the antigravity target was never shown:
// Bug: second arg should be antigravityTarget, not geminiTarget
awfConfigLog.Printf("...overriding gemini target %s with antigravity target %s", geminiTarget, geminiTarget)Rather than fixing the format s…
|
@copilot please run the
|
|
🎉 This pull request is included in a new release. Release: |
Three deprecated
RequiredSubcommandfield usages and one staleGetGeminiAPITargetcall remained inpkg/workflowafter the Gemini→Antigravity rename (80% complete per #44304).RequiredSubcommand→RequiredSubcommands(pip.go,npm.go)The slice-based field takes precedence in
getRequiredSubcommands(). Migrate the three callsites:GetGeminiAPITargetwarning block (awf_config.go)Removes the deprecated call at the old line 516–519 that warned when both
GEMINI_API_BASE_URLandANTIGRAVITY_API_BASE_URLwere set simultaneously. SinceGEMINI_API_BASE_URLis itself deprecated, this warning is no longer useful.Note: the
else iffallback at line 522 (anddomains.go:993) intentionally retainGetGeminiAPITarget— they provide backward compatibility forengine: geminiworkflows as required by ADR-34693. SubstitutingGetAntigravityAPITargetthere would always return""for thegeminiengine, silently breaking proxy target routing for all existing Gemini workflows.