fix(engram): prevent duplicate Antigravity MCP registration#1561
fix(engram): prevent duplicate Antigravity MCP registration#1561dnlrsls wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAntigravity Engram injection now uses plugin-owned MCP registration, migrates legacy global entries, preserves canonical agent arguments, supports Linuxbrew paths, and adds convergence, rollback, malformed-config, and golden-fixture coverage. ChangesAntigravity Engram integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant injectWithOptions
participant installAntigravityEngramPlugin
participant GlobalConfig
participant PluginConfig
participant PluginManifest
injectWithOptions->>installAntigravityEngramPlugin: install Antigravity Engram plugin
installAntigravityEngramPlugin->>GlobalConfig: read and inspect legacy Engram entry
installAntigravityEngramPlugin->>PluginConfig: select command and write canonical Engram config
installAntigravityEngramPlugin->>GlobalConfig: remove legacy registration when migrating
installAntigravityEngramPlugin->>PluginManifest: activate plugin and recover on failure
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Thanks for the focused migration work. I am blocking this because the desktop Antigravity variant can lose its only Engram registration: migration removes the desktop global entry, but the replacement plugin is always written under the CLI path. The operation is also not failure-atomic because the plugin is activated before the global removal, so a failed global write leaves both registrations active. Route the replacement to the selected Antigravity variant and add a write-failure rollback or ordering test, then push an update and I will re-review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/components/engram/inject.go (1)
270-298: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRegistration write happens before its supporting files — partial failure re-creates the duplicate-registration bug.
installAntigravityEngramPluginwritesmcp_config.json(the actual Engram MCP registration) first, thenhooks.json, thenplugin.json. If either later write fails, the caller (injectWithOptions) returns an error before reaching the global-migration step, but the plugin'smcp_config.jsonhas already been committed to disk withengramregistered — while the legacy global entry is still untouched (migration never runs). That leaves Engram registered in both the global config and the plugin config simultaneously, which is precisely the duplicate-registration defect this PR fixes (issue#797), until a subsequent successful run heals it.Writing the registration file last (after the static
hooks.json/plugin.jsoncontent) would ensure a failure never leaves a partial duplicate: no registration exists anywhere until the operation fully completes.♻️ Suggested reordering
func installAntigravityEngramPlugin(pluginDir, engramCommand string) (bool, []string, error) { files := make([]string, 0, 3) changed := false - pluginMCPPath := filepath.Join(pluginDir, "mcp_config.json") - mcpWrite, err := mergeJSONFile(pluginMCPPath, engramOverlayJSON(model.AgentAntigravity, engramCommand)) - if err != nil { - return false, nil, fmt.Errorf("write Antigravity Engram plugin MCP config: %w", err) - } - changed = changed || mcpWrite.Changed - files = append(files, pluginMCPPath) - hooksPath := filepath.Join(pluginDir, "hooks.json") hooksWrite, err := writeFileAtomic(hooksPath, antigravityEngramHooksJSON(), 0o644) if err != nil { return false, nil, fmt.Errorf("write Antigravity Engram hooks: %w", err) } changed = changed || hooksWrite.Changed files = append(files, hooksPath) pluginPath := filepath.Join(pluginDir, "plugin.json") pluginWrite, err := writeFileAtomic(pluginPath, []byte(antigravityEngramPluginJSON), 0o644) if err != nil { return false, nil, fmt.Errorf("write Antigravity Engram plugin manifest: %w", err) } changed = changed || pluginWrite.Changed files = append(files, pluginPath) + pluginMCPPath := filepath.Join(pluginDir, "mcp_config.json") + mcpWrite, err := mergeJSONFile(pluginMCPPath, engramOverlayJSON(model.AgentAntigravity, engramCommand)) + if err != nil { + return false, nil, fmt.Errorf("write Antigravity Engram plugin MCP config: %w", err) + } + changed = changed || mcpWrite.Changed + files = append(files, pluginMCPPath) + return changed, files, nil }As per path instructions, "Install/sync operations must be idempotent (running twice equals running once)" — reruns still converge, but the transient duplicate-registration window during a failed run undercuts the PR's stated goal of registering Engram exactly once.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/components/engram/inject.go` around lines 270 - 298, Update installAntigravityEngramPlugin so the static hooks.json and plugin.json files are written before mcp_config.json. Move the mergeJSONFile call for pluginMCPPath and its error handling after both writeFileAtomic calls, while preserving changed tracking, file ordering, and returned errors.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/components/engram/inject_test.go`:
- Around line 770-814: The hooks.json failure branch in
TestInjectAntigravityMigrationWriteFailuresRemainAtomic does not verify the
transient plugin configuration. Add assertions there to read
pluginDir/mcp_config.json and confirm it contains the expected engram command
and args, matching the existing global-failure branch, while preserving the
current global-file and manifest checks.
---
Outside diff comments:
In `@internal/components/engram/inject.go`:
- Around line 270-298: Update installAntigravityEngramPlugin so the static
hooks.json and plugin.json files are written before mcp_config.json. Move the
mergeJSONFile call for pluginMCPPath and its error handling after both
writeFileAtomic calls, while preserving changed tracking, file ordering, and
returned errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5b00a1db-3d2f-4926-855f-420b6230e5c2
📒 Files selected for processing (2)
internal/components/engram/inject.gointernal/components/engram/inject_test.go
666d02f to
d821fd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/components/engram/inject_test.go (1)
578-589: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
cliMCPPathnow duplicatespluginMCPPath. After this change,cliMCPPath(Line 578) resolves to the exact same file aspluginMCPPath(Line 596) —.gemini/antigravity-cli/plugins/gentle-ai-engram/mcp_config.json— so lines 579-604 read and assert against the same file twice under two different variable names. The distinct naming implies two separate configs and can mislead future maintainers. Consider collapsing into a single read/assert block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/components/engram/inject_test.go` around lines 578 - 589, The test duplicates the same Antigravity MCP configuration read and assertions through cliMCPPath and pluginMCPPath. Consolidate the duplicate read/assert block into one shared path and variable, removing the redundant block while preserving all existing configuration validations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/components/engram/inject.go`:
- Around line 381-390: Restore the Antigravity settings bootstrap before the
staging flow in the relevant injection function, ensuring a missing
settings.json inherits the user’s ~/.gemini/settings.json defaults rather than
only falling back to "{}". Reuse the existing ensureAntigravitySettings helper
and preserve the current files/readImage staging behavior.
---
Outside diff comments:
In `@internal/components/engram/inject_test.go`:
- Around line 578-589: The test duplicates the same Antigravity MCP
configuration read and assertions through cliMCPPath and pluginMCPPath.
Consolidate the duplicate read/assert block into one shared path and variable,
removing the redundant block while preserving all existing configuration
validations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: acfe133e-d6b2-4e5c-afa4-04c6eae1499f
⛔ Files ignored due to path filters (1)
testdata/golden/engram-antigravity-mcp.goldenis excluded by!testdata/**
📒 Files selected for processing (3)
internal/components/engram/inject.gointernal/components/engram/inject_test.gointernal/components/golden_test.go
| files := []string{mcpPath, filepath.Join(pluginDir, "hooks.json"), adapter.SettingsPath(homeDir), manifestPath} | ||
| changed := false | ||
| for i, content := range [][]byte{pluginMCP, antigravityEngramHooksJSON(), []byte("{}\n")} { | ||
| before, readErr := readImage(files[i]) | ||
| if readErr != nil { | ||
| return false, nil, fmt.Errorf("read staging target %q: %w", files[i], readErr) | ||
| } | ||
| if i == 2 && before.exists { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites of ensureAntigravitySettings (excluding its definition)
rg -nP '\bensureAntigravitySettings\s*\(' --type=go -C2
# Confirm whether the ~/.gemini/settings.json bootstrap source is still referenced anywhere
rg -nP '\.gemini/settings\.json|filepath\.Join\([^)]*\.gemini' --type=go -C2Repository: Gentleman-Programming/gentle-ai
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and surrounding symbols first
ast-grep outline internal/components/engram/inject.go --view expanded || true
# Show the relevant region around the staging loop and helper definition
sed -n '340,780p' internal/components/engram/inject.go | cat -n
# Find all references to the helper and the gemini bootstrap source
rg -n --hidden -S 'ensureAntigravitySettings|\.gemini/settings\.json|filepath\.Join\([^)]*\.gemini' internal/components/engram -C 2 || true
# Check whether the helper is called anywhere else in the repo
rg -n --hidden -S '\bensureAntigravitySettings\s*\(' . -g '*.go' -C 2 || trueRepository: Gentleman-Programming/gentle-ai
Length of output: 32556
Keep the Antigravity settings bootstrap
settings.json now falls back to {} when missing, and ensureAntigravitySettings has no production call sites, so Antigravity no longer inherits ~/.gemini/settings.json defaults on first run. Restore that bootstrap before this staging path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/components/engram/inject.go` around lines 381 - 390, Restore the
Antigravity settings bootstrap before the staging flow in the relevant injection
function, ensuring a missing settings.json inherits the user’s
~/.gemini/settings.json defaults rather than only falling back to "{}". Reuse
the existing ensureAntigravitySettings helper and preserve the current
files/readImage staging behavior.
d821fd0 to
42a9787
Compare
|
Maintainer action requested: please apply exactly |
|
Superseded by #1643, which preserves the same issue scope in a smaller, reviewable transaction with the updated recovery coverage. Closing this PR to keep the review queue focused. |
2 participants
🔗 Linked Issue
Closes #797
🏷️ PR Type
type:bug— Bug fix (non-breaking change that fixes an issue)type:feature— New feature (non-breaking change that adds functionality)type:docs— Documentation onlytype:refactor— Code refactoring (no functional changes)type:chore— Build, CI, or tooling changestype:breaking-change— Breaking change (fix or feature that changes existing behavior)📝 Summary
mcp --tools=agent, with recoverable rollback or fix-forward handling for ambiguous write failures.📂 Changes
internal/components/engram/inject.gointernal/components/engram/inject_test.gointernal/components/golden_test.gotestdata/golden/engram-antigravity-mcp.goldenmcp --tools=agentcontract.🧪 Test Plan
Focused unit tests
go test ./...) — deferred to CI✅ Contributor Checklist
status:approvedtype:*label is presentsize:exceptionCo-Authored-Bytrailers📏 Size Exception
The exact GitHub diff is 469 additions + 52 deletions = 521 changed lines across four tightly coupled files. Splitting would separate migration and activation behavior from rollback regressions and the golden contract, so maintainers are asked to apply
size:exception.💬 Notes for Reviewers
Review
installAntigravityEngramPluginfirst for the transaction boundary, thenantigravityConfigsfor migration and selected-variant configuration. Verify failure recovery inTestInjectAntigravityRecoversWriteFailuresand the semantic recovery cases inTestInjectAntigravityManifestWriteFailureUsesSemanticActivity.The intended boundary is limited to Antigravity MCP ownership, migration, activation, and recovery; behavior for other agents is unchanged.
Summary by CodeRabbit
/home/linuxbrew/.linuxbrew/bin/engramas a stable Linuxbrew path.mcp_config.jsonlocation.