refactor(P1): consolidate Antigravity/Gemini engines into shared googleCLIEngine base#46367
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…avity engines - P1.1: computeGeminiToolsCore now delegates to computeGoogleCLIToolsCore via appendBashTools (closes active divergence; previously used inline two-pass logic) - P1.2: new google_cli_engine.go contains: * googleCLIEngineConfig – all per-engine constants in one place * googleCLIEngine – shared base embedding BaseEngine * appendBashTools / computeGoogleCLIToolsCore – shared utility functions Shared methods on googleCLIEngine (promoted to both engines via embedding): GetModelEnvVarName, GetSupportedEnvVarKeys, GetRequiredSecretNames, GetSecretValidationStep, GetDeclaredOutputFiles, GetAgentManifestFiles, GetAgentManifestPathPrefixes, GetPreBundleSteps, RenderMCPConfig, ParseLogMetrics, GetLogParserScriptId, generateSettingsStep, GetExecutionSteps - gemini_engine.go: now only NewGeminiEngine + GetInstallationSteps - antigravity_engine.go: now only NewAntigravityEngine + GetInstallationSteps - gemini_tools.go / antigravity_tools.go: thin wrappers for tests - Removed gemini_mcp.go, antigravity_mcp.go, gemini_logs.go, antigravity_logs.go - Updated test calls from generateGeminiSettingsStep / generateAntigravitySettingsStep to the unified generateSettingsStep Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No new test functions were added or modified in this PR. Changes were refactoring only - method calls updated from generateAntigravitySettingsStep/generateGeminiSettingsStep to generateSettingsStep in existing test cases. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Consolidates duplicated Gemini and Antigravity engine behavior into a shared Google CLI engine base.
Changes:
- Centralizes execution, configuration, MCP, logging, and tool mapping.
- Retains engine-specific installation and constants.
- Updates tests to exercise shared settings generation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/google_cli_engine.go |
Adds the shared engine implementation. |
pkg/workflow/gemini_engine.go |
Reduces Gemini to configuration and installation. |
pkg/workflow/gemini_tools.go |
Delegates tool mapping to the shared helper. |
pkg/workflow/gemini_mcp.go |
Removes superseded MCP implementation. |
pkg/workflow/gemini_logs.go |
Removes superseded log implementation. |
pkg/workflow/gemini_engine_test.go |
Updates shared settings method calls. |
pkg/workflow/antigravity_engine.go |
Reduces Antigravity to configuration and installation. |
pkg/workflow/antigravity_tools.go |
Delegates tool mapping to the shared helper. |
pkg/workflow/antigravity_mcp.go |
Removes superseded MCP implementation. |
pkg/workflow/antigravity_logs.go |
Removes superseded log implementation. |
pkg/workflow/antigravity_engine_test.go |
Updates shared settings method calls. |
.github/workflows/agentic-auto-upgrade.yml |
Changes the generated weekly schedule. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Medium
| if cmdStr == "*" || cmdStr == ":*" { | ||
| log.Print("bash wildcard → run_shell_command") | ||
| return append(toolsCore, "run_shell_command") |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: googleCLIEngine consolidation
Clean refactor. Behavioral equivalence verified:
mirrorAPIKeyAsmirrors ANTIGRAVITY_API_KEY after all env overrides, matching old inline behaviorextraAllowedSecretsin Antigravity config preserves the oldappend([]string{"GEMINI_API_KEY"}, ...)pattern forFilterEnvForSecretssort.Strings(toolsCore)ordering preservedGH_AW_MCP_CONFIGpath viae.cfg.configDirmatches old hardcoded pathsGetID()resolves to correct engine name for AWF- Test method renames match promoted shared method
Non-blocking: googleCLIEngineConfig mixes a *logger.Logger with config constants; unconventional but consistent with existing codebase patterns.
~1,000 lines removed, key two-pass vs. single-pass bash-mapping divergence resolved, no behavioral regressions found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 45.2 AIC · ⌖ 4.34 AIC · ⊞ 5K
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (735 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on test coverage gaps and one type-safety issue.
📋 Key Themes & Highlights
Issues Found
-
Test gap —
appendBashToolswildcard-after-specific discard (line 171): The documented behavior that a wildcard appearing after specific commands silently discards those commands is untested. A loop refactor could silently regress this. -
Test gap —
mirrorAPIKeyAsinGetExecutionSteps(line 571): The Antigravity-specificANTIGRAVITY_API_KEY → GEMINI_API_KEYmirroring is the most critical behavioral difference between the two engines and has no test coverage. A regression would silently break the Gemini proxy sidecar. -
GetSupportedEnvVarKeysmissing mirrored key (line 239): Antigravity'sGEMINI_API_KEYis not declared as a supportedengine.envkey, so strict-mode validation would reject a user override for it even though the engine explicitly sets it. -
Unsafe
constants.EngineNamecast (line 444):constants.EngineName(e.GetID())replaces the previous compile-time-safeconstants.AntigravityEngine/constants.GeminiEngineconstants. Adding anengineName constants.EngineNamefield to the config struct would restore the safety.
Positive Highlights
- ✅ Excellent elimination of ~90% code duplication — the config-struct approach is clean and extensible
- ✅
mirrorAPIKeyAsrunning afterengine.envoverrides is the right ordering - ✅
appendBashToolssingle-pass logic and comment are clear - ✅ Test rename from
generateAntigravitySettingsStep→generateSettingsStepis correct; shared-method tests continue to validate both engines
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 74 AIC · ⌖ 5.66 AIC · ⊞ 6.7K
Comment /matt to run again
| } | ||
| if cmdStr == "*" || cmdStr == ":*" { | ||
| log.Print("bash wildcard → run_shell_command") | ||
| return append(toolsCore, "run_shell_command") |
There was a problem hiding this comment.
[/tdd] Missing test: wildcard-after-specific discards prior entries, but this behavior is untested.
The comment on line 151 explicitly documents that a wildcard appearing after specific commands silently discards those commands. This non-obvious semantic should be covered by a regression test.
💡 Suggested test skeleton
t.Run("wildcard after specific discards prior entries", func(t *testing.T) {
log := logger.New("test")
result := appendBashTools(nil, []any{"grep", "*"}, log)
assert.Equal(t, []string{"run_shell_command"}, result,
"wildcard anywhere should collapse to run_shell_command")
})Without this, the discard-semantics could silently regress if the loop changes.
@copilot please address this.
| if e.cfg.mirrorAPIKeyAs != "" { | ||
| if _, alreadySet := env[e.cfg.mirrorAPIKeyAs]; !alreadySet { | ||
| env[e.cfg.mirrorAPIKeyAs] = env[e.cfg.apiKeySecretName] | ||
| } |
There was a problem hiding this comment.
[/tdd] The mirrorAPIKeyAs behavior in GetExecutionSteps is not covered by any test.
When Antigravity engine runs, ANTIGRAVITY_API_KEY must be mirrored into GEMINI_API_KEY — but the tests only touch generateSettingsStep, not GetExecutionSteps. A regression here would silently break the Gemini proxy sidecar.
💡 Suggested test approach
Add a test in antigravity_engine_test.go that calls NewAntigravityEngine().GetExecutionSteps(workflowData, "/tmp/log") and asserts the resulting step YAML contains both ANTIGRAVITY_API_KEY and GEMINI_API_KEY environment entries, and that their values match.
@copilot please address this.
|
|
||
| // GetSupportedEnvVarKeys returns the engine.env variable names this engine supports. | ||
| func (e *googleCLIEngine) GetSupportedEnvVarKeys() []string { | ||
| return []string{e.cfg.apiKeySecretName} |
There was a problem hiding this comment.
[/codebase-design] GetSupportedEnvVarKeys returns only the primary API key secret, but Antigravity also needs GEMINI_API_KEY to be overridable via engine.env (it mirrors the primary key into that variable).
If a user sets GEMINI_API_KEY in engine.env to override the mirrored value, strict-mode validation will reject it because GEMINI_API_KEY is not in the supported keys list — even though the engine explicitly supports it.
💡 Suggested fix
Either add mirrorAPIKeyAs to GetSupportedEnvVarKeys when non-empty:
func (e *googleCLIEngine) GetSupportedEnvVarKeys() []string {
keys := []string{e.cfg.apiKeySecretName}
if e.cfg.mirrorAPIKeyAs != "" {
keys = append(keys, e.cfg.mirrorAPIKeyAs)
}
return keys
}Or document explicitly that the mirrored key is intentionally not overridable via engine.env.
@copilot please address this.
| allowedDomains = workflowData.CachedAllowedDomainsStr | ||
| } else { | ||
| allowedDomains = GetAllowedDomainsForEngine( | ||
| constants.EngineName(e.GetID()), |
There was a problem hiding this comment.
[/codebase-design] constants.EngineName(e.GetID()) is an unsafe string-to-type cast — it bypasses the constants-based type system.
GetID() returns a raw string from BaseEngine.id, and casting it to constants.EngineName will produce a valid-looking but unknown engine name if the ID is ever inconsistent with the constants. The previous code used constants.AntigravityEngine / constants.GeminiEngine directly, which is safe at compile time.
💡 Suggested fix
Add a engineConstantName() constants.EngineName field or method to googleCLIEngineConfig (populated in NewGeminiEngine / NewAntigravityEngine) and use that here instead of the cast. This keeps the constants-based type safety.
@copilot please address this.
There was a problem hiding this comment.
Non-blocking observation
The consolidation is clean — ~1,100 lines deleted, 12 file pairs collapsed into one shared base, behavioral parity well-preserved.
One low-severity finding (see inline comment):
Runtime EngineName cast in GetExecutionSteps
The old per-engine GetExecutionSteps implementations passed the typed constant (constants.GeminiEngine, constants.AntigravityEngine) to GetAllowedDomainsForEngine. The new shared implementation casts the runtime e.GetID() string instead. This works correctly today, but a typo/rename of BaseEngine.id would silently fall through to the wrong (default) domain allow-list without any compile-time signal. Storing constants.EngineName in googleCLIEngineConfig or adding a unit-test assertion would close the gap.
🔎 Code quality review by PR Code Quality Reviewer · 106.6 AIC · ⌖ 4.46 AIC · ⊞ 5.6K
Comment /review to run again
| allowedDomains = workflowData.CachedAllowedDomainsStr | ||
| } else { | ||
| allowedDomains = GetAllowedDomainsForEngine( | ||
| constants.EngineName(e.GetID()), |
There was a problem hiding this comment.
Runtime string cast bypasses type-safe engine name lookup: constants.EngineName(e.GetID()) casts the BaseEngine.id string to constants.EngineName at runtime instead of using the pre-declared constants constants.GeminiEngine / constants.AntigravityEngine.
💡 Why this matters
GetAllowedDomainsForEngine uses the EngineName to key into engine-specific domain allow-lists. The old engine implementations hard-coded the typed constant (constants.GeminiEngine, constants.AntigravityEngine); the refactored version casts the runtime id string instead. This means:
- A typo or future rename of
BaseEngine.idsilently produces an unknownEngineName, falling through to the default domain list without a compile-time failure. - A new engine embedding
googleCLIEnginewith a non-standardidgets the wrong domain allow-list with no warning.
Consider adding engineName constants.EngineName to googleCLIEngineConfig and using it directly, or at minimum adding a test assertion that constants.EngineName(e.GetID()) matches the expected constant.
|
@copilot please run the Unresolved review feedback:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ted schedule change Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…t implementation Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed both unresolved threads in adeb58c:
Local |
The Antigravity engine was a near-verbatim clone of the Gemini engine across 5 file pairs (~90–95% identical), accumulating behavioral drift. Most critically,
computeGeminiToolsCoreused an inline two-pass bash-mapping loop whilecomputeAntigravityToolsCorealready used the canonical single-passappendBashToolshelper — a live divergence in behavior.Changes
New
google_cli_engine.go— shared base parameterized by per-engine constants:googleCLIEngineConfig— all engine-specific values in one place: API key name, CLI binary, config dir, CLI flags, env var names, step labels, file globs, log parser name, API-key mirroring, etc.googleCLIEngine— embedsBaseEngine; provides 13 methods promoted to both engines via Go embedding:GetExecutionSteps,generateSettingsStep,GetRequiredSecretNames,RenderMCPConfig,ParseLogMetrics, and moreappendBashTools/computeGoogleCLIToolsCore— logger-parameterized shared utilities replacing both the inline Gemini logic and the Antigravity-only helperEngines reduced to their unique parts:
gemini_engine.go→NewGeminiEngine()+GetInstallationSteps()(npm)antigravity_engine.go→NewAntigravityEngine()+GetInstallationSteps()(GCS binary)gemini_tools.go/antigravity_tools.go→ thin wrappers preserving existing test function signaturesDeleted:
gemini_mcp.go,antigravity_mcp.go,gemini_logs.go,antigravity_logs.go— all methods now live ongoogleCLIEngine.The
mirrorAPIKeyAsfield onAntigravityEngine's config handles the existing behavior whereANTIGRAVITY_API_KEYis mirrored intoGEMINI_API_KEYfor the Gemini proxy sidecar — applied after allengine.envoverrides so the mirror tracks the effective key value.