feat(connector-gemini): add Gemini CLI harness connector#507
feat(connector-gemini): add Gemini CLI harness connector#507
Conversation
|
Hi @aaf2tbz - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the feature work in |
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
0fa1a8e7
Harness output could not be parsed as structured JSON.
You're out of extra usage · resets 10pm (America/Denver)
[stderr]
SessionEnd hook [signet hook session-end -H claude-code] failed: /bin/sh: line 1: signet: command not found
Integrates Signet with Gemini CLI by: - Writing GEMINI.md to ~/.gemini/ with composed identity - Registering Signet hooks (SessionStart, SessionEnd, BeforeAgent, PreCompress) - Registering Signet MCP server under mcpServers in settings.json - Configuring context.fileName to include AGENTS.md - Symlinking skills to ~/.gemini/skills/ New package @signet/connector-gemini follows the established connector pattern from connector-opencode/connector-claude-code. Registered in CLI setup wizard, sync detection, harness type, and build system.
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: gpt-5.3-codex | commit:
0fa1a8e7
[Automated Review by pr-reviewer] Found blocking correctness issues in Gemini connector config mutation/uninstall behavior, plus a PR-description/implementation divergence.
Confidence: High [sufficient_diff_evidence, targeted_context_included, missing_runtime_repro] - The findings are directly visible in changed code paths in packages/connector-gemini/src/index.ts (hook filtering/removal and uninstall behavior) and can be reasoned about statically from the new logic. I did not run Gemini CLI end-to-end, so runtime event-name validation is not reproduced here.
- Add @signet/connector-gemini workspace dependency to @signet/cli - Bump connector-gemini version from 0.98.15 to 0.98.16 for version alignment - Update bun.lock
0fa1a8e to
1489c24
Compare
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: gpt-5.3-codex | commit:
1489c243
[Automated Review by pr-reviewer] Found blocking correctness issues in Gemini connector uninstall/hook mutation behavior, plus one PR-description divergence.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - The issues are directly visible in packages/connector-gemini/src/index.ts: uninstall() never removes the managed skills symlink despite install() creating it, and both configureHooks()/removeHooks() drop entire hook groups when any hook name starts with 'signet-'. The PR description also claims isHarnessInstalled checks ~/.gemini directory existence, but code checks ~/.gemini/settings.json.
…s symlink - Filter individual signet hooks within groups instead of dropping whole groups - Remove skills symlink during uninstall - Align isHarnessInstalled with PR description (check ~/.gemini/ not settings.json) - Add regression tests for mixed hooks and skills symlink cleanup (8 tests)
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: gpt-5.3-codex | commit:
61b7a592
[Automated Review by pr-reviewer] Found a blocking uninstall data-loss risk in the Gemini connector.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - In the new uninstall path, the code unconditionally deletes ~/.gemini/skills if it exists, without verifying it is the Signet-managed symlink. This is directly visible in the changed diff and can remove user-owned Gemini skills content.
- Only remove symlinks inside skills dir, never real user directories - Guard with lstatSync().isSymbolicLink() before unlinkSync - Remove empty skills dir only after all signet symlinks cleaned - Remove unused symlinkSync and lstatSync imports from test file - Add regression test for user-managed real skills directory (9 tests)
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: gpt-5.3-codex | commit:
ce946b21
[Automated Review by pr-reviewer] Found a blocking uninstall data-loss issue in Gemini skills cleanup logic.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - The uninstall path in packages/connector-gemini/src/index.ts now unlinks every symlink entry under ~/.gemini/skills without checking whether each symlink was Signet-managed. This is directly visible in the changed loop and can remove user-managed symlinked skills.
- Record skills source path in settings.json[signet.skillsSource] during install - Uninstall only removes symlinks whose target starts with recorded source - Preserves user-owned symlinks and real directories inside ~/.gemini/skills - Add regression test for non-signet symlink preservation (10 tests)
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: gpt-5.3-codex | commit:
3630e088
[Automated Review by pr-reviewer] Found one blocking data-safety issue in Gemini skills uninstall ownership checks.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - In packages/connector-gemini/src/index.ts, uninstall ownership is decided by target.startsWith(signetSkillsSource) after readlinkSync. This prefix check is not path-boundary-safe, so symlinks targeting sibling paths like /path/skills-backup can be misclassified as Signet-owned and unlinked.
- Use exact match or separator check instead of raw startsWith - Prevents false-positive deletion of /x/skills-backup when source is /x/skills - Add regression test for prefix-colliding symlink paths (11 tests)
Summary
Adds
@signet/connector-gemini— a new harness connector enabling Signet to install into Google's Gemini CLI via hooks, MCP server configuration, context file injection, and extension setup.Changes
packages/connector-gemini/src/index.ts— FullGeminiConnectorimplementation:install(): configures hooks (SessionStart, SessionEnd, BeforeAgent, PreCompress) insettings.json, injects Signet MCP server, symlinks skills to~/.gemini/skills/, composes identity extras intoGEMINI.md, setscontext.fileNameto includeAGENTS.md, records skills source insettings.json[signet.skillsSource]uninstall(): strips Signet hook entries (preserving non-signet hooks within same groups), removes MCP server, removes only signet-owned skill symlinks (verified via target path matching recorded source), preserves user-owned symlinks and real directories, cleans identity blocksisInstalled(): checks for Signet hook presence in settingsisHarnessInstalled(): checks for~/.gemini/directory existenceatomicWriteJson(),stripSignetBlock(),generateHeader(),composeIdentityExtras(),symlinkSkills()from@signet/connector-basepackages/connector-gemini/index.test.ts— 10 tests covering install round-trip, uninstall, isInstalled, signet skill symlink cleanup, user directory preservation, non-signet symlink preservation, mixed hook preservationpackages/connector-gemini/package.json+tsconfig.json— Package scaffoldingpackages/core/src/identity.ts— Addedgemini: booleantoSetupDetection.harnessesinterfacepackages/cli/package.json— Added@signet/connector-geminiworkspace dependencypackages/cli/src/cli.ts— AddedGeminiConnectorimport + case inconfigureHarnessHooks()switchpackages/cli/src/features/setup-shared.ts— Added "gemini" toHarnessChoicetype,SETUP_HARNESS_CHOICES,formatDetectionSummary()packages/cli/src/features/sync.ts— AddedGeminiConnectordetection indetectHarnesses()package.json(root) — Added@signet/connector-geminitobuild:depsfilterAGENTS.md— Added package map entry for@signet/connector-geminiType
feat— new user-facing feature (bumps minor)Packages affected
@signet/connector-*@signet/cli/ dashboardPR Readiness (MANDATORY)
INDEX.md+dependencies.yaml)Testing
bun testpassesbun run typecheckpassesbun run lintpassesAI disclosure
Assisted-bytags in commits)Notes
New harness connector following the established pattern. No migrations, no spec changes, no API changes.
Review Rounds
Round 1 — sqmd-review (local, pre-push)
"gemini"toextractionProvidersFromHarnesses()condition, but"gemini"is not a valid providersync.tsRound 2 — CI failures (post-push)
0.98.15while signetai at0.98.160.98.16, rebased onto latest main@signet/connector-gemininot in@signet/clipackage.jsonworkspace:*dependencyRound 3 — @PR-Reviewer-Ant (commit
1489c243)configureHooks()/removeHooks()remove entire groups if any hook name starts withsignet-, losing non-signet hooks in same groupindex.ts:239.map()filtering individual hooks within groups instead of dropping whole groupsuninstall()never removes~/.gemini/skillssymlinkindex.ts:95uninstall()isHarnessInstalledchecks~/.gemini/but code checked~/.gemini/settings.jsonindex.ts:141isHarnessInstalled()to check~/.gemini/directory existenceRound 4 — @PR-Reviewer-Ant + github-code-quality[bot] (commit
61b7a592)symlinkSyncandlstatSyncunused in test fileindex.test.ts:2-3uninstall()usedrmSync({recursive:true})on~/.gemini/skillswhich could delete user-managed real directoryindex.ts:117unlinkSync, never real directoriesRound 5 — @PR-Reviewer-Ant (commit
ce946b21)uninstall()removed any symlink inside~/.gemini/skillsregardless of ownershipindex.ts:122settings.json[signet.skillsSource]during install; uninstall only removes symlinks whose target starts with the recorded source pathRound 6 — sqmd-review (local, with updated skill, pre-push)
Verdict: no_issues. Convention checklist passed. Historical patterns passed. 10 tests passing.