fix(engram): recover Antigravity plugin migration#1643
Conversation
📝 WalkthroughWalkthroughAntigravity Engram injection now installs configuration through a dedicated plugin, reconciles and migrates existing state with rollback support, preserves agent-tool arguments, recognizes Linuxbrew paths, and expands tests for convergence, path handling, failures, and golden configuration output. ChangesAntigravity Engram integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Injection
participant EngramPluginInstaller
participant AntigravityFiles
Injection->>EngramPluginInstaller: install or reconcile Engram plugin
EngramPluginInstaller->>AntigravityFiles: read global and plugin configuration
EngramPluginInstaller->>AntigravityFiles: write MCP config, hooks, and manifest
AntigravityFiles-->>EngramPluginInstaller: write or activation result
EngramPluginInstaller-->>Injection: changed files or recovery error
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
374-440: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRestore staged plugin files when activation fails.
Lines 376-392 write
mcp_config.json,hooks.json, and possiblysettings.jsonbefore migration/activation. If manifest activation fails pre-replacement, Lines 425-427 restore only the global config; the staged files remain modified. In particular, an existinghooks.jsoncan be overwritten despite a failed install.Capture and restore every staged before-image (deleting newly created files) on the rollback path, and add recovery assertions for those files. As per path instructions, “Any write to user config must have a backup/restore 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 374 - 440, Update the staging loop in the activation flow to retain each target’s before-image for mcpPath, hooks.json, and settings.json. When manifest activation fails before replacement, restore every staged file through the existing reconciliation mechanism, deleting files that did not previously exist, and include recovery results/assertions for those restores in the returned error alongside the global rollback status. Ensure any restore failure reports manual recovery rather than leaving staged user configuration modified.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.
Outside diff comments:
In `@internal/components/engram/inject.go`:
- Around line 374-440: Update the staging loop in the activation flow to retain
each target’s before-image for mcpPath, hooks.json, and settings.json. When
manifest activation fails before replacement, restore every staged file through
the existing reconciliation mechanism, deleting files that did not previously
exist, and include recovery results/assertions for those restores in the
returned error alongside the global rollback status. Ensure any restore failure
reports manual recovery rather than leaving staged user configuration modified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 077610cc-fb47-4c4e-abaa-1967d72771b6
📒 Files selected for processing (2)
internal/components/engram/inject.gointernal/components/engram/inject_test.go
1 participant
🔗 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
📂 Changes
internal/components/engram/inject.gointernal/components/engram/inject_test.gointernal/components/golden_test.gotestdata/golden/engram-antigravity-mcp.golden--tools=agentprofile.🔗 Chain Context
This PR supersedes the oversized #1602 without rewriting its published history.
🧪 Test Plan
Focused tests
Go Format
go run ./internal/gofmtcheck)go test -count=1 ./...completed locally — exceeded the local execution allowance with unrelated environment-dependent failurescd e2e && ./docker-test.sh)🤖 Automated Checks
status:approvedtype:*Labeltype:bug✅ Contributor Checklist
status:approvedtype:buglabelCo-Authored-Bytrailers💬 Notes for Reviewers
Please focus on the registration invariant across pre-replacement, post-replacement, rollback, and roll-forward outcomes. A pre-existing manifest counts as active only when it exactly matches the desired plugin manifest.
Summary by CodeRabbit