fix(mcp): register Claude Code Context7 MCP in ~/.claude.json - #1909
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClaude Code MCP injection now writes Context7 and Engram registrations to ChangesClaude Code MCP configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Injector
participant ClaudeUserConfig
participant LegacyMCPFiles
participant Uninstaller
Injector->>ClaudeUserConfig: merge Context7 and Engram registrations
Injector->>LegacyMCPFiles: remove legacy managed files
Uninstaller->>ClaudeUserConfig: rewrite managed MCP entries
Uninstaller->>LegacyMCPFiles: remove remaining legacy files
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/components/mcp/inject_test.go (1)
481-533: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a file-mode assertion for
~/.claude.json.The injector deliberately writes
0600because this file carries the OAuth session, but no test locks that in — a future change toWriteFileAtomicor the mode argument would silently widen it to0644. The seed file here is already written0600, so the check is cheap.🔒 Assert the restrictive mode survives injection
raw, err := os.ReadFile(userConfigPath) if err != nil { t.Fatalf("ReadFile(user config) error = %v", err) } + info, err := os.Stat(userConfigPath) + if err != nil { + t.Fatalf("Stat(user config) error = %v", err) + } + if mode := info.Mode().Perm(); mode != 0o600 { + t.Fatalf("~/.claude.json mode = %O; want 0600 (holds the OAuth session)", mode) + }🤖 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/mcp/inject_test.go` around lines 481 - 533, Extend TestInjectClaudeWritesUserConfigAndIsIdempotent to stat ~/.claude.json after injection and assert its permission mode remains 0600. Use the existing userConfigPath symbol and fail the test with the observed mode if the file is missing or has broader permissions.
🤖 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 57-79: Add engram-side test coverage for the strict parse guard in
Inject, mirroring the existing corrupt-user-config refusal test. Create a
corrupt ~/.claude.json, call Inject with claudeAdapter(), assert it returns an
error, then verify the file remains byte-identical to the original corrupt
bytes.
In `@internal/components/engram/inject.go`:
- Around line 320-343: Extract the duplicated Claude user-config merge flow into
a shared filemerge helper, such as MergeIntoClaudeUserConfig, preserving the
existing read, strict JSON-parse guard, error semantics, overlay merge, and
atomic 0600 write behavior. Replace the inline logic in the ClaudeCode branch of
inject.go and injectClaudeUserConfig in the MCP injector with calls to this
helper, retaining each caller’s changed-state and file-list handling.
In `@internal/components/mcp/inject.go`:
- Around line 299-302: Update injectClaudeUserConfig around the
osReadFile(settingsPath) call so settings.json read failures are treated as
best-effort cleanup failures rather than returned to the caller. Preserve the
successful ~/.claude.json registration and continue without attempting the
inert-block cleanup, matching the existing tolerance for unparsable settings
content.
In `@internal/components/uninstall/service.go`:
- Line 741: The Claude configuration rewrites in uninstall must preserve the
existing 0600 permissions of the OAuth-session file. Update both rewriteJSONFile
calls in internal/components/uninstall/service.go at lines 741 and 792-794 to
preserve the existing mode or explicitly use 0600 for
claude.UserConfigPath(homeDir); apply the same change at both sites.
---
Outside diff comments:
In `@internal/components/mcp/inject_test.go`:
- Around line 481-533: Extend TestInjectClaudeWritesUserConfigAndIsIdempotent to
stat ~/.claude.json after injection and assert its permission mode remains 0600.
Use the existing userConfigPath symbol and fail the test with the observed mode
if the file is missing or has broader permissions.
🪄 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: b5884d7d-82f9-402d-80e3-df452cf881c1
📒 Files selected for processing (6)
internal/agents/claude/adapter.gointernal/components/engram/inject.gointernal/components/engram/inject_test.gointernal/components/mcp/inject.gointernal/components/mcp/inject_test.gointernal/components/uninstall/service.go
|
Los cuatro hallazgos eran validos y estan resueltos en
Nota de tamano: el diff paso de 400 exactas a 430 con estos cambios pedidos por el review. Si prefieren que lo baje recortando comentarios aviso, o queda a criterio de ustedes el |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/agents/claude/adapter.go`:
- Around line 101-112: Serialize the full read-merge-write sequence around raw
file loading, JSON merging, and WriteFileAtomic in the adapter’s injection
method. Use a shared cross-process lock (or equivalent conflict/retry mechanism)
so concurrent injections cannot merge from stale content or overwrite
registrations; preserve the existing validation and error handling within the
protected operation.
In `@internal/components/mcp/inject_test.go`:
- Around line 510-514: Update the fixture setup in the injection test to create
~/.claude.json with insecure 0644 permissions, then retain the existing
post-injection assertion in the relevant test to require 0600 on non-Windows
systems. Ensure the test verifies tightening an existing file rather than merely
preserving already-secure permissions.
🪄 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: 66e1ae29-a1bb-4d35-b43c-03ac62a15823
📒 Files selected for processing (6)
internal/agents/claude/adapter.gointernal/components/engram/inject.gointernal/components/engram/inject_test.gointernal/components/mcp/inject.gointernal/components/mcp/inject_test.gointernal/components/uninstall/service.go
|
Ronda 2, un fix y una declinacion razonada: Permisos (aplicado): el fixture ahora siembra Lock cross-process (declinado, con razones): la carrera read-merge-write que señalas es real pero preexistente y sistemica — cada |
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/mcp/inject_test.go (1)
499-505: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert byte-level idempotency.
The test only checks
second.Changed == false; it never verifies that the second invocation leaves~/.claude.jsonunchanged. Capture the file after the first injection and compare it byte-for-byte with the contents after the second injection so rewrites that incorrectly reportChanged == falsecannot pass.As per path instructions, install/sync operations must be idempotent (running twice equals running 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/mcp/inject_test.go` around lines 499 - 505, Update the idempotency test around the second Inject call to capture the ~/.claude.json bytes immediately after the first injection, then read the file again after the second call and compare the contents byte-for-byte. Retain the existing second.Changed assertion while ensuring the second invocation produces exactly the same file bytes as the first.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/mcp/inject_test.go`:
- Around line 499-505: Update the idempotency test around the second Inject call
to capture the ~/.claude.json bytes immediately after the first injection, then
read the file again after the second call and compare the contents
byte-for-byte. Retain the existing second.Changed assertion while ensuring the
second invocation produces exactly the same file bytes as the first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b0fa078-5803-407e-909f-97dfaf9d8c51
📒 Files selected for processing (1)
internal/components/mcp/inject_test.go
|
Audited PR #1909 against linked Issue #1868 ( |
|
Aplicada la asercion de idempotencia byte a byte del test (comentario fuera-de-diff de la ronda 3). Gracias @decode2 por la auditoria — queda listo para el review final. |
dnlrsls
left a comment
dnlrsls
left a comment
There was a problem hiding this comment.
Three issues need resolution before merge:
-
internal/agents/claude/adapter.go:101-113performs an uncoordinated read/merge/rename of~/.claude.json. This PR makes both Context7 and Engram write that shared file, so concurrent runs can read the same base and last-writer-wins, dropping one registration. A concurrent Claude Code update can also be overwritten. Please serialize the complete operation or add file-identity conflict detection with retry. -
internal/components/filemerge/writer.go:48-52returns before applying the requested mode when the merged bytes are unchanged. Consequently,claude.MergeUserConfig(..., 0600)can leave a byte-identical OAuth-bearing~/.claude.jsonat0644. Please enforce0600on the unchanged-content path and add coverage for an already-correct but loosely permissioned config. -
The current diff is 439 lines (
+368/-71)..github/workflows/pr-check.ymlcounts raw additions plus deletions with a 400-line limit, and this PR has nosize:exception; PR Validation will fail once the approval-gated workflows run. Please split the change or obtain the required exception.
Focused tests for the four affected packages, gofmt, and go vet pass locally. The hosted CI/PR Validation workflows have not run at this head because they are still action_required.
|
@dnlrsls los tres puntos, resueltos o encaminados: 1. Concurrencia (aplicado): 2. Modo en el camino no-op (aplicado): buen hallazgo — 3. Tamano: el diff quedo en 499 (+428/−71) — los +139 sobre el presupuesto vienen integramente de las 4 rondas de review (helper compartido, preservacion de modos en uninstall, retry de concurrencia, y 5 tests de seguridad nuevos). Partirlo ahora separaria las dos mitades del mismo invariante (#1868 documenta ambas y el helper es el punto unico de enforcement de las dos). Pido formalmente el Suites de los 4 paquetes afectados + gofmtcheck verdes en local (Windows 11 amd64, go1.26.2). |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/agents/claude/adapter.go`:
- Around line 106-112: Update the config-loading flow around readUserConfigBase
and the UnmarshalJSONObject validation so a nonexistent user config is treated
as an empty merge base and allows initialization. Preserve rejection of existing
empty or malformed files by performing JSON validation only when the base file
exists, using an existence flag or equivalent distinction from
readUserConfigBase, and apply the same behavior to the other referenced call
site.
- Around line 117-127: Protect the complete read-merge-write sequence containing
readUserConfigBase, the bytes.Equal retry check, and filemerge.WriteFileAtomic
with a shared cross-process lock, held through the write and released on every
return path. Ensure all writers use the same lock so no process can modify the
configuration between validation and atomic replacement.
🪄 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: 860037ca-5ff6-4442-8ba6-ad8bf90e81d2
📒 Files selected for processing (2)
internal/agents/claude/adapter.gointernal/components/mcp/inject_test.go
| raw, err := readUserConfigBase(configPath) | ||
| if err != nil { | ||
| return filemerge.WriteResult{}, configPath, err | ||
| } | ||
| if _, parseErr := filemerge.UnmarshalJSONObject(raw); parseErr != nil { | ||
| return filemerge.WriteResult{}, configPath, fmt.Errorf("refusing to modify %q: it holds the Claude Code session and could not be parsed as JSON: %w", configPath, parseErr) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Allow a missing user config to initialize the merge.
readUserConfigBase returns nil, nil for a nonexistent file, but UnmarshalJSONObject(nil) fails. First-time injection therefore aborts instead of creating ~/.claude.json—including TestInjectClaudeTightensModeOnNoOpRun.
Return an existence flag from readUserConfigBase, or only validate JSON when a base file exists; continue rejecting an existing empty or malformed file.
Also applies to: 141-146
🤖 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/agents/claude/adapter.go` around lines 106 - 112, Update the
config-loading flow around readUserConfigBase and the UnmarshalJSONObject
validation so a nonexistent user config is treated as an empty merge base and
allows initialization. Preserve rejection of existing empty or malformed files
by performing JSON validation only when the base file exists, using an existence
flag or equivalent distinction from readUserConfigBase, and apply the same
behavior to the other referenced call site.
dnlrsls
left a comment
dnlrsls
left a comment
There was a problem hiding this comment.
This PR is not safe to merge yet. Three lifecycle defects need correction: (1) install verification and backup wiring still target the legacy Claude MCP paths instead of the new ~/.claude.json registry; (2) uninstall routes the user registry through cleanup that can delete the whole file when the last managed server is removed; and (3) the read/re-read/atomic-replace sequence can still lose a concurrent Claude or installer update between the final read and rename. Add focused regression coverage for all three. The PR is also 499 changed lines without size:exception, its body is missing required template sections, and the current head lacks the required hosted CI run. Please split below budget or obtain the explicit exception, repair the body, and rerun all required checks.
|
Gracias por la review — los tres defectos eran reales. Fixes pusheados y verificados, uno por uno:
Ademas, al correr la suite completa de Una aclaracion honesta sobre el punto 3: el lock cierra la carrera entre escritores de gentle-ai (install, sync, upgrade, uninstall — todos cooperan). Claude Code no toma ningun lock propio sobre su archivo, asi que contra ese escritor la unica defensa posible sigue siendo el re-read/retry dentro de la seccion critica — la ventana queda minima pero no se puede eliminar sin cooperacion del otro proceso. Si conocen un mecanismo que Claude Code respete, lo sumo. Sobre el proceso:
|
dnlrsls
left a comment
dnlrsls
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier lifecycle findings. The verification/backup wiring and non-deleting uninstall path are now present, but the current head still has three merge-blocking correctness gaps:
-
internal/cli/run.go:1342-1343,1747-1753,1953-1957: workspace-scoped Claude Engram routes and verifies<workspace>/.claude.json. Claude Code does not use that file for user MCP registration, so the command can report success while Engram remains unavailable. Keep workspace prompt delivery separate from the Claude user MCP registry, and merge/verify the supported registry location. -
internal/agents/claude/adapter.go:125-145: the final compare followed by atomic replacement still has a TOCTOU window for non-cooperating Claude Code writes. Claude can refresh OAuth/session state after the comparison and before rename, and the stale merged file then replaces it. Please use Claude's supported configuration command or an identity-bound compare-and-replace that refuses stale writes. The advisory lock only coordinates Gentle AI processes. -
internal/components/communitytool/tool.go:177-200andinternal/components/communitytool/codegraph_contract.go:88-92: the Claude-targeted CodeGraph installer writes the same~/.claude.jsonregistry withoutclaude.LockUserConfig. Concurrent CodeGraph and Context7/Engram operations can still lose one registration. Route every Gentle AI writer through the same serialization boundary and add a concurrent regression test.
Two additional lifecycle gaps should be covered in the same correction: legacy Context7 cleanup at internal/components/mcp/inject.go:292-301 identifies ownership by server name alone and can delete a user-authored entry, and the 0600 guarantee is not enforced on Windows because os.Chmod does not establish a restrictive DACL while the regression test skips Windows.
Focused Claude/MCP/Engram/uninstall and CLI path/backup/idempotency tests pass. The PR still has 858 changed lines without size:exception, and only CodeRabbit is reported at this head, so the required hosted checks must also run before merge.
654f507 to
1679991
Compare
Denver2828
changed the title
1679991 to
024e08f
Compare
|
@dnlrsls tomé la salida que venias señalando: split. Este PR quedo recortado al nucleo de context7.. rebaseado sobre main post-v2.2.2, 400 lineas exactas.. y el resto del trabajo queda mapeado en PRs chicos encadenados. El branch completo con todo lo revisado en estas 6 rondas existe y esta probado; nada se pierde, solo cambia el empaquetado. Que queda en ESTE PR: registracion de context7 en Roadmap de lo demas, incluyendo tus 3 puntos de la ultima review:
Sobre el ownership del cleanup (tu otro punto): tenias razon en settings.json.. el bloque se removia con chequear solo el nombre Con esto el PR cumple el budget sin exception. Cuando puedan, disparen los workflows del fork para este head.. cualquier rojo lo atiendo enseguida. |
|
Los tres issues de diseño del roadmap ya estan abiertos:
Con esto, cada punto de la ultima review tiene su casa: los arreglados aca, los que van en los PRs B y C de la cadena, y los de diseño en sus issues. |
Claude Code only reads user-scope MCP servers from ~/.claude.json. Register Context7 there with a non-destructive merge (corrupt base aborts, 0600 enforced, stale merges re-read and retry), clean the inert settings.json block best-effort, verify the registry post-install, and never delete the user registry on uninstall even when it empties. Closes Gentleman-Programming#1868
024e08f to
e5220fb
Compare
|
Maintainer rebase: I rebased this branch onto current main to resolve the conflict with #1617 (scoped Context7 injection). Two adjustments were needed to keep both fixes working together:
Full |
Alan-TheGentleman
added
the
size:exception
|
Gracias por el rebase, Alan.. lo verifique de mi lado y esta todo verde. Verificacion en Windows 11 (mi maquina, el entorno donde nacio el bug):
El split que elegiste.. Una sola cosa que dejo anotada sin re-litigarla aca, porque le abri issue aparte: en scope workspace la registracion sigue quedando en un archivo que Claude Code no lee para MCP de user scope.. es el #2007. Si hay un motivo por el que ese path si sirve, cerramos el issue y listo; y si no, se discute ahi con calma sin frenar este PR. Dato para tu decision: el rebase dejo el diff en 487 lineas (venia en 400 exactas despues del split). Las ~87 extra son la resolucion del confilcto con #1617, no scope nuevo mio. Decime si preferis que intente recortarlo, o si con tu propio rebase el tema del presupuesto queda saldado. Lo que sigue de mi lado, ya construido y probado, para abrir cuando este mergee: engram al registry (lo adapto a la firma nueva de |
3603f22
into
Gentleman-Programming:main
4 participants
🔗 Linked Issue
Closes #1868
🏷️ PR Type
type:bug.. Bug fix (non-breaking change that fixes an issue)📝 Summary
Claude Code solo lee registraciones MCP de user scope desde
~/.claude.json. Hoy gentle-ai escribe context7 en~/.claude/settings.json(clavemcpServersque Claude Code ignora en ese archivo), asi que el server instalado nunca aparece en Claude Code.Este PR es el primer paso de un split acordado en la review (ver roadmap en el hilo): registra context7 en
~/.claude.jsoncon un merge no destructivo.. base corrupta aborta (nunca resetea el archivo que carga la sesion OAuth), retry si la base cambia debajo, modo 0600 forzado siempre.. limpia best-effort el bloque inerte de settings.json, apunta la verificacion post-install al registry real, y el uninstall nunca borra el archivo aunque quede vacio. Engram, el lock cross-proceso y el resto del lifecycle vienen en los PRs siguientes del roadmap.📂 Changes
internal/agents/claude/adapter.goUserConfigPathyMergeUserConfig(parse estricto, retry ante base movida, 0600 incondicional)internal/components/mcp/inject.go~/.claude.json; limpieza best-effort del bloque inerte en settings.jsoninternal/components/uninstall/service.gomcpServers.context7en el registry viarewriteClaudeUserConfig, que nunca borra el archivo y preserva el modointernal/cli/run.go~/.claude.json(el write target real)🧪 Test Plan
Unit Tests
go test ./...Go Format
E2E Tests (Docker required)
go test ./...)go run ./internal/gofmtcheck)cd e2e && ./docker-test.sh).. corren en CI; sin Docker en mi maquina WindowsPrueba manual: instale gentle-ai v2.2.0 en mi maquina y el sync re-creo el bloque inerte en settings.json; con este branch la registracion queda en
~/.claude.jsony Claude Code la lee.✅ Contributor Checklist
status:approvedsize:exceptionwith rationale documented.. 400 exactas (364+36) tras el splittype:*label to this PRgo test ./...)go run ./internal/gofmtcheck)cd e2e && ./docker-test.sh).. via CICo-Authored-Bytrailers💬 Notes for Reviewers
Rebaseado sobre main post-v2.2.2 y recortado al nucleo de context7 siguiendo el pedido de split de la review. El roadmap completo del split (engram, lock advisory + CodeGraph, backup/#1794, y los issues de diseño para workspace-scope, TOCTOU contra Claude Code y ACLs de Windows) esta detallado en el comentario de respuesta a la ultima review.