feat(skills): add Antigravity as a verified host, retire Gemini CLI, stop trusting the journal over the filesystem - #22
Conversation
The journal and report fixtures used `gemini-cli` as their stand-in host: a string key in one, an unverified-host literal in the other. Neither resolves through the registry, so both kept passing after the host was removed while naming an assistant the CLI no longer supports. Repointed at github-copilot, which is a real registry entry and genuinely unverified, so the fixtures keep meaning what they say.
ArtemKosolap
left a comment
ArtemKosolap
left a comment
There was a problem hiding this comment.
Read this against the registry and the journal rather than from the description, and ran it: 177 passed on this head. Two of the three things in it are good, and the third is the one I would not merge without a sentence somewhere.
What I verified by planting the defect each change exists to catch:
entry_files_presentis genuinely covered — replacing it withtruefails exactly the new repair test. It is also a real bug fix worth having:installshort-circuiting tocurrentover an empty directory, writing nothing, exiting 0, and being unable to repair what it just called healthy is about the worst shape a fix-it command can take.delete_filesreportingoutsideonly for paths that exist is right, and the new "already gone" test covers it: there is nothing to delete and nothing to warn about.canonical()is not covered — see the inline note.
Retiring gemini-cli orphans anything installed under it, and nothing says so
This is the finding I care about. The journal is keyed by host id (hosts: Record<host, Record<scope, Record<pack, entry>>>), nothing iterates journal branches that are not in HOSTS, and host_by_id now throws Unknown assistant 'gemini-cli'. So for anyone who installed while Gemini CLI was detected:
- the files stay in
~/.gemini/skills, - the journal keeps a branch nothing will ever read,
reply skills listwill not mention them,reply skills removecannot take them out, and--agent gemini-cliis now a usage error.
The host was never verified, but the installer wrote to it anyway — that is the documented behaviour for an unverified host — so these are real files on real disks. And remove taking packs out is a promise the README makes plainly.
Cheapest fix is one line in the README: if you installed for Gemini CLI, delete ~/.gemini/skills by hand, because this version no longer knows that host. The durable version, worth considering because this will happen again, is list reporting journal branches whose host is unknown to the current registry — that turns a silent orphan into a line the user can act on, for every future retirement rather than this one.
The title and the commit message hide the part that deserves the most attention
The PR reads add Antigravity as a verified host, retire Gemini CLI, and the commit says includes minor bug-fixes. What actually changed is install and list no longer trusting the journal over the filesystem, and path canonicalisation inside the code that decides which files may be deleted. Neither is minor and neither is incidental to the host swap. A reviewer going by either line would skim exactly the hunks where a mistake costs the most. Worth splitting, or at least retitling so the diff announces itself.
Antigravity needs no new session; the report still asks for one
report.ts:124 prints Start a new session in each assistant so the skills load whenever anything changed, unconditionally, while both READMEs now say Antigravity re-reads its skills every turn. Last round the same mismatch was a wording choice; here it is a behavioural difference, and the tool asks for something the host does not need. A per-host activation note in the registry would settle it for good — the data is already per host.
Everything else lines up: the registry order change is harmless because the tests derive expected order from the registry, the .agents/skills overlap paragraph correctly grows to Cursor, Codex and Antigravity, and the coverage sentence counts down to GitHub Copilot properly.
Three review findings from the Antigravity change, none of them about the host swap itself. `list` now reports journal branches whose host is not in the registry. Retiring a host leaves its packs unreachable: every path iterates the registry, and `--agent <retired id>` is a usage error, so `remove` cannot take those files out and nothing ever said so. Anyone who installed while Gemini CLI was detected has a full set of skills on disk with no way to find that out. Reading the journal directly makes the leftovers visible for this retirement and every future one, without depending on someone writing a release note for the specific host. `needs_new_session` moves into the registry, and the reporter consults it. The "start a new session" line went to everyone whenever anything changed, while Antigravity re-reads its skills every turn — so the tool asked its users for something the host does not need. A run touching both kinds now names the hosts that need it rather than generalising. `canonicalise` becomes an injectable dependency of the flat adapter, the same seam detect.ts uses for `exists`. The case-insensitivity fix had no portable coverage: each platform-specific test is skipped on the other platform, so swapping `fs.realpathSync.native` for `path.resolve` left the suite green. Two tests now supply a lower-casing canonicaliser and assert the logic — our own file recognised through a case difference, a foreign file still refused — and two more pin the default via a symlink, which realpath follows and resolve does not. Also states in hosts.ts why detection keys on .gemini/antigravity rather than .gemini: the latter belonged to the retired host, so matching it detected Gemini CLI on every machine running Antigravity and wrote skills where nothing reads them. The app creates that directory on first launch, before anything skills-related, so it is present on a machine that has never run this CLI.
CaptainReply
changed the title
|
All four addressed in Retired hosts are now reported, not just documentedYou were right that this is the one that mattered, and a README line would have been the weaker half of it — so That is the version you called durable: it works for the next retirement without anyone remembering to write a note for the specific host, and it points at a real path rather than describing one. Reported on I had written the README line you suggested, and it was cut on the author's call — the argument being that Gemini CLI was never verifiable, so the page should not carry an obituary. Reporting it in the tool satisfies your point without that paragraph, which is why this shape rather than both.
|
4ae58b0 to
f5fa4e3
Compare
…es it ours CI caught a disagreement between two checks added in the previous commit. `owns_dir` asks the canonicaliser whether two spellings name one file; `entry_files_present` asked `fs.existsSync` about the raw path. On a case-insensitive filesystem both said the file was there, on a case-sensitive one ownership said "ours" while presence said "missing" — so an identical re-copy reported `current` on macOS and `installed` on Linux, and the ubuntu job failed where the windows job passed. Presence now consults the canonical form too, raw path first so the ordinary case resolves nothing. A test pins the branch that only a case-sensitive runner reached: the recorded path exists nowhere as written, and the injected canonicaliser is the only thing mapping it to the file that does, so any platform fails if that branch goes away.
f5fa4e3 to
3dfa158
Compare
ArtemKosolap
left a comment
ArtemKosolap
left a comment
There was a problem hiding this comment.
Everything from the last round is in, and the orphan finding got the version I hoped for rather than the cheap one — list reading the journal directly and naming packs recorded for a host the registry no longer has. That covers this retirement and every future one without anyone remembering to write a release note.
3dfa158 is a good catch by the CI: owns_dir asking the canonicaliser while entry_files_present asked fs.existsSync about the raw path meant an identical re-copy reported current on macOS and installed on Linux. Worth noting the two checks now share one rule.
I ran the suite on this head, and there is one thing to fix before merge, plus one small gap.
The test that pins canonical() cannot run on an ordinary Windows account
The comment above it is right about what it is for — the injected-canonicaliser tests do not exercise the default, and swapping fs.realpathSync.native for path.resolve leaves them green. But "portably" does not hold: creating a symlink on Windows needs Developer Mode or an elevated shell, and without either fs.symlinkSync throws.
On my machine, a clean clone of this branch gives:
FAIL src/__tests__/skills/adapter-flat.test.ts > canonical > answers with the path the filesystem really uses, following a symlink
Error: EPERM: operation not permitted, symlink 'C:\...
eply-flat-bUcHnJ
eal-skills' -> 'C:\...\linked-skills'
553 passed, 1 failed. CI is green because the GitHub Windows runner allows symlink creation; a contributor's machine typically does not, so a fresh clone looks broken and the failure has nothing to do with their change.
It also removes the signal the test exists for. I planted the defect — path.resolve in place of fs.realpathSync.native — and on this machine the suite fails identically before and after, because the one test that would have noticed never reaches its assertion.
This file already has the idiom: CASE_INSENSITIVE_FS and it.skipIf at lines 743 and 761. Probing symlink support once and skipping the same way keeps it running where it can. Worth adding a case-insensitive assertion beside it too — canonical handed a differently-cased spelling of a real path should return the on-disk one — so the default stays pinned on the platforms where the symlink case is skipped.
needs_new_session: false for Antigravity is not pinned anywhere
Flipping it to true keeps the whole suite green. The three reporter tests are the right tests and they do their job — they exercise the logic with explicit fixtures — but nothing asserts the registry value itself, and "the one host that needs nothing" is a claim both READMEs now make to users.
hosts.test.ts is where this belongs: it already pins the verified set with marks only the hosts we actually verified. One more line in the same shape would pin which hosts ask for a restart.
|
Holding approval on one thing only: the symlink test skipping rather than failing where symlinks are not permitted. To be clear about why that one and not the other two — it is not the inconvenience, it is that the test stops being a test. On a Windows account without Developer Mode the run never reaches the assertion, so on that platform The file already has the shape for it at lines 743 and 761. A probe plus The other two can follow whenever, or in a separate change — pinning Ping me when it is pushed and I will re-run and approve. Then this one and the release before the docs change lands, same order as last time. |
fs.symlinkSync throws EPERM on a Windows account without Developer Mode or an elevated shell, so the symlink case failed there rather than skipping. That left a red test on a clean clone and, worse, no coverage of the default canonicaliser on that platform: swapping fs.realpathSync.native for path.resolve produced an identical suite result. It now skips on Windows, where a differently-cased path pins the same default instead — NTFS folds case, so realpath answers with the spelling on disk while path.resolve hands back the one asked for. Neither case runs everywhere; between them every platform keeps one that does. Also pins needs_new_session in the registry test, in the shape the verified-set test already uses, since nothing said which host is the exception.
|
Pushed as One deviation from what you asked, with the reason. You suggested a probe plus Neither case runs everywhere, which is why both are there:
I re-ran your experiment against the result —
Separately: GitHub Copilot is now verified end to end, which closes the last of the four hosts. It is #23, stacked on this branch, rather than more commits here — this PR is what you are mid-review on. Everything about it lives there, including a session that called a pack and the per-pack sequence read back from |
ArtemKosolap
left a comment
ArtemKosolap
left a comment
There was a problem hiding this comment.
Approving. Re-ran on the machine the fix was for — Windows, no Developer Mode, clean clone of this head:
- 555 passed, 5 skipped, 0 failed. It was 553 with one red before.
- Planted the defect again:
fs.realpathSync.native→path.resolvefailsanswers with the spelling on disk, not the one asked for. The default is genuinely covered on this platform now, which it was not. - Planted
needs_new_session: trueon Antigravity:marks only the hosts that pick up skills without a new sessionfails. Working tree back to identical after both.
Your deviation is the right call and I would not want the probe. With the case assertion in place the only machine it changes anything for is a Developer-Mode Windows box, and that box keeps coverage through the case test anyway — so twelve lines and filesystem I/O at import would buy running a redundant case. The table in your comment is the argument, and it holds: between the two, every platform keeps one that runs.
Retitling the PR to name the journal change is worth as much as the code. A reviewer skimming add a host, retire a host would have gone past the hunks where a mistake costs most.
Order as before: this one, then a release, then the docs change — that page describes a flag that only reaches users with the release. And noting for whoever picks up #23: verifying the last host empties the unverified list, so the guard added here fires by design and someone has to decide what those two tests assert next. That was the point of making it fail loudly, so it is arriving on purpose rather than going quiet.
2 participants
Stacked on #21 — review that one first, or merge it and GitHub will retarget this to
main.Antigravity replaces Gemini CLI as the Google host we support, plus the bug fixes that verification turned up.
Antigravity — verified end to end, both scopes
Antigravity 2.0.1. User scope
~/.gemini/config/skills, project scope.agents/skills.Nothing here rests on files sitting in the right folder. Every user-scope number below comes from asking the host itself through its language server's
GetAllSkillsRPC, and both scopes were confirmed by real sessions:Analyzed sending-guardrails/SKILL.md #L1-299in the UI, and the transcript carries the file'sTODO(expert)markers — 10 in the file, 20 in the transcript, i.e. the body read twice. Those strings exist nowhere but inside the file~/ag-ws-probe/.agents/skills, 0 to the user pathcore→ 9,+adapter→ 14,+runtime→ 18, at both scopesupdatecurrent, no re-copy--dry-run, support filestemplates/plan.md,references/workspace-spec.mdIt needs nothing to take effect. Unlike every other host, Antigravity re-reads its skills each turn: a conversation opened while nothing was installed picked up all 18 after an install, with no restart and no new chat. Verified from a transcript that began listing 46 skills and later listed ours.
Why a plain skills directory and not a plugin
The first implementation made a plugin —
~/.gemini/config/plugins/reply-skills/with aplugin.json— because that is what I found by reading the language server's strings, and it worked (18/18, session-verified). Then I read the host's own bundledagy-customizationsskill, which documents two customization roots, and~/.gemini/config/skillsneeds no manifest, no plugin, and no new registry field. Both are documented and first-class; the plain directory is the one that keeps this host identical in shape to Cursor.That reverted a
Host_def.skills_manifestfield, its write logic and three tests. If we ever want to ship anmcp_config.jsonor hooks alongside the skills — a plugin can bundle those, a loose directory cannot — the plugin form is where that goes, and it would earn the field then.Also worth recording:
ScanSkillsConfigFileandGetSkillMarketplaceLinkreturn "only available in Google environments", and there is noantigravitybinary anywhere. There is no install command to drive; copying files is the only route, sokindstaysflat-skills-dir.Gemini CLI — removed
Signing in now fails with "this client is no longer supported for Gemini Code Assist for individuals" and a redirect to Antigravity. We could never open a session, so we could never confirm it reads what we install, and it has been unverified through two releases.
Removing it also deletes a real bug rather than filing it: detection is "does the config directory exist", and Gemini CLI was keyed on
.gemini— which Antigravity creates. Any Antigravity-only machine reported Gemini CLI, and a plainreply skills installwrote 18 skill directories into~/.gemini/skillswhere nothing reads them.hosts.test.tsnow asserts no host's config directory contains another's; re-adding the old entry makes it fail withantigravity inside gemini-cli.One consequence to be aware of: packs installed there by 0.5.1 are no longer reachable by
remove, since the registry is what it iterates. The README says to delete~/.gemini/skillsby hand. Happy to add a migration path instead if you'd rather.The journal-trust fix
installdecided its work from the journal alone — entry exists, complete, version matches — and returned before touching the filesystem. Delete your skills directory and:listsaid the same. So the one command a user reaches for to fix a broken install was the one that could never fix it. This bit us three times during this ticket, once badly enough that recovery meant hand-editing~/.config/reply/skills.json: when the recorded paths sit outside the current target root,removerefuses to delete outside it (correct) and keeps the entry (deliberate) — and that same entry short-circuitsinstall. Deadlock with no CLI way out, which is what any future change to a host's skills directory would have caused.Four changes, all in
adapter-flat.ts:entry_files_present— does the entry's file list still exist on diskinstall'spendingfilter consults it, so a stale entry no longer short-circuits the copylistconsults it too, reusing the existing incomplete-install message, which now names a command that worksdelete_filesrefuses an outside-the-root path only if it is really there — nothing absent can be stranded by forgetting it, and that breaks the deadlockA repair also had to report
installed, notcurrent: the version had not moved, socopied_outcomecalled itcurrent, which is the exact phrasing that caused the confusion. An entry whose files are gone no longer counts as a previous install, captured before the copy since the copy records a fresh entry.The macOS case failure
treats it as already ours where the filesystem ignores casehas been failing on macOS and is invisible on CI, because it isskipIf(!CASE_INSENSITIVE_FS)and Linux skips it.owns_dircompared paths withpath.relative, which is case-insensitive on Windows but not on POSIX — so on macOS a journal entry recording a different case than the disk made the install report a conflict against its own file and refuse to overwrite it. The comment onis_withinasserted the opposite, which is why it went unfixed.Fixed with a
canonical()helper (fs.realpathSync.native, falling back topath.resolvefor paths that do not exist), used by the ownership check only. Containment stays strict: refusing to delete a path we cannot prove is inside the root is the safe direction, and loosening it via symlink-resolvingrealpathwould weaken a security guard to fix a cosmetic bug.This one is unrelated to the ticket — say the word and I will split it out.
Verification
545 passing, 1 skipped, no failures — the first fully clean run of this suite on macOS in this ticket.
Still open, not in this PR
remove corecan never succeed. The guard walks the registry rather than what is installed, so it refuses even withreply-adapteralready removed (host loading 9 skills, core alone). Reproduced live.reply-skillscontent bug. With the skill demonstrably in context, three of three hosts asserted30–50/dayand2–4 weekfigures thatsending-guardrailswithholds behindTODO(expert), and recommended Instantly, Smartlead, MillionVerifier and ZeroBounce — zero occurrences of any of them in the pack.TODO(expert)reads to a model as an invitation to fill the gap.