Repository navigation
Conversation
7f624d9 to
d40d150
Compare
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at exact head d40d150d9654b2c6d3696944bd295bae18547082. I checked it out in a detached worktree.
What I verified:
- Rename integrity:
skills/agy/scripts/{agy-browser.sh,gemini-run.sh}are pure renames (+0/−0).skills/agy/SKILL.mdis the old body under the new name. The manifest isagyat 2.0.0. - No stale references:
git grep claude-geminioutsideCHANGELOG.mdand the stub dir returns nothing. The router (claude-router/SKILL.md,route-ai.sh) and both tests point atskills/agy/. - Alias stub:
skills/claude-gemini/SKILL.mdkeeps/claude-geminiinvocable. With arguments it runs$SKILL_DIR/../agy/scripts/gemini-run.sh, which resolves becauseinstall.shlinks both skill dirs side by side. - Tests at head:
tests/agy-browser.test.pypasses 29/29 andtests/agy-wrapper.test.pyis OK. - Review checks:
scripts/review-checks.sh --diff <gh pr diff>from the PR-head tree gives PASS (hardcoded-paths, root-artifacts, prose-cap).
Non-blocking, merge order: #4966 (25e88cae7, open) edits skills/claude-gemini/SKILL.md, which this PR turns into the stub. Once #4966 lands, this PR has to be rebased so its content moves into skills/agy/SKILL.md. Otherwise the conflict resolution can silently keep the stub and drop #4966's edit. After the rebase, please confirm skills/agy/SKILL.md carries #4966's change. That push makes this approval stale, so I'll re-check then.
At review time CI had 2 checks still pending; this approval doesn't certify them.
|
CI at |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/docs/claude-gemini-installer-and-cdp-note...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
|
@qingyun-wu reminder at Chi's request: CODEOWNERS ( |
keweichen
left a comment
There was a problem hiding this comment.
Requesting changes on d40d150d9654b2c6d3696944bd295bae18547082.
Blockers
-
The compatibility alias does not survive the normal existing-install upgrade path. The PR body says an existing
claude-geminisymlink keeps the old slash command working before the user rerunsskills/install.sh, but the stub invokes the sibling installed path at$SKILL_DIR/../agy, andrefresh-skill.sh --allcannot create a newagylink. The router also now hard-requires that absent sibling atskills/agy/scripts/gemini-run.sh.I reproduced a pre-rename install with the four existing symlinks, then ran the documented refresh:
refresh-skill: skills dir /private/tmp/pr4994-upgrade.TjisqP refreshed claude-gemini refreshed claude-codex refreshed claude-router refreshed claude-pi done — updated skills are live in the running session (no restart). agy_link_after_refresh=no router_after_refresh_rc=1 route-ai.sh: missing Gemini wrapper: /private/tmp/pr4994-upgrade.TjisqP/agy/scripts/gemini-run.sh legacy_direct_path_rc=127 bash: /private/tmp/pr4994-upgrade.TjisqP/claude-gemini/scripts/gemini-run.sh: No such file or directoryWorst case: an existing user pulls and refreshes successfully, but both the promised
/claude-geminialias and the Gemini branch of/claude-routerstop working until they know to rerun the installer; any saved prompt/cron using the previously documented direct script path also breaks immediately. Please preserve/migrate the old installed and direct paths for the deprecation release (or make the router/stub safely resolve the shipped source), and add a regression test that starts from the pre-rename symlink layout. The current tests only execute the new source-tree paths, so they cannot detect this. -
The declared parent is not incorporated yet. PR #4966 head
25e88cae7da3718ae296c93d0b9ec2f19c1312dais not an ancestor of this head, andgit merge-treereports a conflict inskills/claude-gemini/SKILL.md. Consequently the currentskills/agy/SKILL.mdstill has the pipe-to-shell installer and lacks #4966's unauthenticated-CDP warning. After #4966 lands, rebase and explicitly carry both of its safety edits intoskills/agy/SKILL.md; then refresh the before/after evidence and rerun the full checks on the new SHA. This head cannot be the final reviewed tree for the declared stack.
Also required in the update
git diff --check 1be758ba5461b9d365f9e3d74ce83e2cae2964a0...HEADcurrently fails withskills/agy/SKILL.md:133: new blank line at EOF.skills/claude-router/manifest.jsonremains stable1.0.0and still advertises the Gemini CLI even though this change replaces its required sibling withagy. Please update the package metadata consistently with the breaking dependency change. The copied wrapper comment atskills/agy/scripts/gemini-run.sh:112-113likewise still says the manifest is1.0.0, whileagy/manifest.jsonis2.0.0.- Fix the PR evidence when refreshing it: the after wrapper block currently has a blank
exit=, and the install proof runsls -l $T/skillswithout ever assigningT.
Verified positives on this exact head: both moved scripts are byte-identical and remain executable; agy-wrapper passed 6/6, agy-browser passed 29/29, the router suite passed 5/5, strict lint passed all 58 manifests, review-checks.sh passed, and the current CI rerun plus CLA are green. Those do not exercise the existing-install migration above.
d40d150 to
ff64448
Compare
eed62d3 to
d4b41ea
Compare
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head d4b41ea647c244e165a0295074c2d0e2f19f39fa from a detached worktree. Everything the update claims to fix checks out, but restacking onto #4966 dropped another merged fix. This replaces my d40d150d9 approval.
Blocker: #4965's agy-browser fix (0c03031b0) is no longer in this PR.
git merge-base --is-ancestor 0c03031b0 d40d150d9succeeds, and the same check againstd4b41ea64fails. #4966's branch forks from main before #4965, so restacking on it lost that commit. That's why compared40d150d9...d4b41ea64showsbehind_by: 4.- Line by line, four of #4965's five non-blank added lines in
agy-browser.share missing fromskills/agy/scripts/agy-browser.shat head: the two comment lines,set +u; launcher="${launcher:-$!}"; set -u, and[[ -n "$launcher" ]] || { rm -rf "$HS"; return 0; }. The fifth (local st pg rc) only matches an unchanged line elsewhere, so the block is gone. tests/agy-browser.test.pyalso lost #4965's 63 test lines. It runs 27/27 at head versus 29/29 atd40d150d9.git merge-tree origin/main d4b41ea64conflicts onskills/claude-gemini/scripts/agy-browser.sh. Resolving it by keeping the shim would shipskills/agy/scripts/agy-browser.shwithout the fix, silently.
Fix: rebase the stack onto current main (#4966 first, or this PR after #4966 lands). Then confirm #4965's block is in skills/agy/scripts/agy-browser.sh and the test file is back to 29.
Verified OK at head:
- Installer snippet. It is byte-identical to #4966
219d4694b: the pagerifis at line 29,printf …; read -r aat line 30, and the CDP note at line 63. Neither skill dir containsread -r -panywhere. - Path resolution. The shims and
route-ai.shresolve paths withcd … && pwd -Pand quote every path.- From a skills dir under
sp ace/App Support, both shims and the router resolved and ran. - The arguments
"a b" "" '$c' -- dreached the target unchanged, viaexec bash "$target" "$@".
- From a skills dir under
- Upgrade test.
agy-rename-upgrade-pathpasses 4/4 at head. Copied tod40d150d9, it fails at its first assertion (claude-gemini/scripts/gemini-run.sh: No such file or directory). Changing the router'spwd -Ptopwdmakestest_router_resolves_wrapperfail. - Manifests.
agyis 2.0.0 andclaude-router2.0.0.git grep claude-geminioutside CHANGELOG and the stub only hits the new test.review-checks.sh --diffgives PASS.
Nit: the upgrade test doesn't pin the shims' -P. Swapping it to pwd in claude-gemini/scripts/gemini-run.sh still passes, because the OS resolves ../.. through the real directory.
…he CDP port has no auth Two review nits from #4908. The Setup step piped a remote installer straight into bash; it now saves the script to a file to read first, since the installer publishes no version pin or checksum to reference. The Browser section says what CDP implies: any local process can drive the agy Chrome through 127.0.0.1:9222, which is why the port is loopback-only and the browser has its own profile. Closes #4915 Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…mes --port Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…; drop the temp file `less` always exits 0, so `less "$f" && bash "$f"` ran the installer whatever the reader saw, hung an agent without a TTY in the pager, and left the mktemp file behind. The step now pages only on a TTY (prints otherwise), runs the installer only after `y`, and removes the temp file. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…pager branch `read -r -p` is bash-only: zsh (the macOS default) fails with `-p: no coprocess`, leaves `$a` empty, and the step deleted the installer without running it. The prompt is now `printf` + `read -r`, which both shells accept; the pager line is an explicit if/else so a non-zero `less` cannot fall through to `cat`. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ed; always remove the temp file A failed or truncated curl (rc=18 when Content-Length exceeds the body) did not stop the later lines, so a partial file was shown and run after `y`. The step is now a subshell with an EXIT trap: review, prompt and run happen only inside `if curl …; then`, and the temp file is removed on every exit. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ff67c01 to
aa359df
Compare
d4b41ea to
bad6e52
Compare
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at exact head bad6e52f1f1d6fd548bd09722267540c861ed7ed. This clears my CHANGES_REQUESTED from d4b41ea64.
The restack blocker is fixed:
git merge-base --is-ancestor 0c03031b0 bad6e52f1succeeds, so #4965 is back.aa359df3a(#4966) is also an ancestor.skills/agy/scripts/agy-browser.shat head is byte-identical toorigin/main:skills/claude-gemini/scripts/agy-browser.sh: both are bloba339ce4c70a3.git merge-tree --write-tree origin/main bad6e52f1is clean.
Tests at head (detached worktree):
tests/agy-browser.test.py: PASS 29/29, back from 27.tests/agy-wrapper.test.py: OK.tests/agy-rename-upgrade-path.test.py: OK.
My pwd -P nit: both shims now resolve here with pwd -P (skills/claude-gemini/scripts/{agy-browser,gemini-run}.sh:4). I didn't re-run your test_shims_name_the_shipped_tree mutation myself.
Everything from my previous pass still holds: the installer block matches #4966, path resolution handles spaces, and arguments pass through intact.
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed the current head bad6e52f1f1d6fd548bd09722267540c861ed7ed after the branch moved during review. The previous stack, installer, browser-fix, and canonical upgrade-path blockers are fixed. Two compatibility-test blockers remain, so this head is not merge-ready.
Blockers
-
The new upgrade suite fails from a checkout path containing spaces. That is the normal deployed shape here (
Application Support), not an exotic path. In an exact-head worktree namedcheckout with space,python3 tests/agy-rename-upgrade-path.test.pypasses its first two cases, then fails attests/agy-rename-upgrade-path.test.py:69-71:command: bash /private/tmp/.../checkout\ with\ space/skills/agy/scripts/gemini-run.sh --cd /private/tmp/.../checkout\ with\ space --approval-mode plan -- x AssertionErrorThe production router returned rc 0 and emitted correct shell-escaped
%qoutput; the test's\S+regex cannot parse it.exec_target()at line 77 makes the same whitespace assumption over bash xtrace and fails next when called directly. Parse the shell quoting (for example withshlex.split) or assert behavior without reparsing display text, and run this suite from a spaced repo path as a regression case. -
The claimed installed-sibling fallback is neither functional nor exercised. The shim first resolves the physical source tree with
pwd -P; on a miss, line 6 buildsinstalled/claude-gemini/scripts/../../agy. For a symlinked alias, pathname resolution followsclaude-geminibefore.., so that second candidate still lands in the physical tree.Exact fixture:
installed/claude-gemini -> physical/claude-gemini, nophysical/agy, and a validinstalled/agysibling. The current shim traces both candidates and exits 1 withskills/agy not found ... or the installed dir. The copied-skill test never reaches the fallback: its first physical candidate already is the copied directory's sibling. As a mutation control, replacing the fallback assignment with a definitely missing path leaves all six upgrade cases green.Either implement the installed-root calculation the way the router does (perform
cd .../../..beforepwd) and pin the true fallback layout, or remove the dead branch/fallback promise and describe the supported copied-directory case accurately.
Verified fixed / passing
bad6e52fdirectly parents current #4966 headaa359df3; the curl-gated installer and #4965's browser cleanup fix are carried.agy-browser.shis byte-identical to the parent path and its renamed suite passes 29/29.- The real pre-rename symlink upgrade path now works: old direct scripts,
/claude-gemini, and router Gemini routing all resolve the shippedagytree. The previousd40d150d9tree fails the same regression at the first direct-path assertion. - Upgrade suite 6/6 on a no-space path, wrapper 6/6, router 5/5, strict manifest lint 58/58,
git diff --check, andreview-checks.shpass. - The current required Python 4/5 check is also red from
agy-start-cli.test.py's unrelated race mutation control not firing. That still needs a green rerun before merge; it does not explain either blocker above.
Verdict: changes requested. I can re-check once the two path/fallback cases are corrected and current required CI plus CLA are green.
In zsh 5.9 a subshell's EXIT trap does not run when Ctrl-C interrupts `read` at the "Run the installer?" prompt, so the downloaded script stayed in $TMPDIR. Trap INT, TERM and HUP as well; the file is then removed in both an interactive zsh and `zsh -c`. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bad6e52 to
a17ac4d
Compare
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approving at exact head 4b84691c39cc8bcb6d28a93a31255761cf58eeca, which sits on #4966's 63d0407a6.
- Installer snippet:
skills/agy/SKILL.mdhas the Setup block byte-identical to #4966 at63d0407a6(diffof the extracted blocks is empty), signal traps included. - Ancestry:
0c03031b0(#4965) is still an ancestor.skills/agy/scripts/agy-browser.shis still the same blob asorigin/main:skills/claude-gemini/scripts/agy-browser.sh. - Delta since my
bad6e52f1approval:- Both shims now build the fallback as
cd "$(dirname …)/../.." && pwdfollowed by/agy/scripts/…. That is the logical installed skills directory, so a symlinked sibling layout resolves. - The error message names the installed skills dir.
- The upgrade-path test was extended to pin this (+84/−48 across 4 files).
- Both shims now build the fallback as
- Tests at head (detached worktree):
agy-rename-upgrade-pathOK,agy-wrapperOK,agy-browserPASS 29/29.
CI was still running at review time (15 pass, 9 pending). This approval doesn't cover the pending lanes.
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact current head 4b84691c39cc8bcb6d28a93a31255761cf58eeca, stacked directly on #4966 head 63d0407a6. The two blockers from my bad6e52f1 review are fixed: paths containing spaces pass, and the installed-sibling fallback works in direct probes. One regression-test blocker remains.
Blocker — the installed-sibling test covers only one of the three fallback consumers. tests/agy-rename-upgrade-path.test.py:110-120 invokes only the legacy gemini-run.sh shim. It never drives the same installed-root decision in:
skills/claude-gemini/scripts/agy-browser.sh:6; orskills/claude-router/scripts/route-ai.sh:38-40.
Both implementations work at this head in direct fixtures. But the committed suite is not capable of detecting their regression:
- changing only the browser shim fallback to
/nonexistent/...still produced all fivePASSnames plusOK, rc 0; - changing only the router fallback to
SKILLS_DIR=/nonexistentdid the same.
That leaves two adjacent instances of the exact compatibility bug this update fixes unpinned. Please extend the installed-sibling fixture to exercise both shims (agy-browser.sh status --help is sufficient) and the router's --dry-run --engine gemini path, assert their installed-tree targets, and show each corresponding mutation fails.
Everything else checked out: the upgrade suite's five named cases pass; wrapper 6/6 and browser 29/29 pass; the parent is exact #4966 head, #4965 is an ancestor, and the browser implementation is byte-identical to main's parent path. Strict skill lint, git diff --check, cumulative review checks, all CI lanes, and CLA are green. Worst case of the uncovered branches is that an existing installation loses the promised old browser or router path during the one-release compatibility window.
REQUEST_CHANGES solely for the two unexercised installed-sibling paths; no current production defect was reproduced.
The skill drives the Antigravity CLI `agy`; its old name described the standalone gemini CLI Google folded into Antigravity. Move the skill to skills/agy (frontmatter name, title and usage follow; manifest name agy, version 2.0.0), point claude-router at the new path (manifest 2.0.0, its required sibling changed), rename the two tests, and keep skills/claude-gemini as a one-release alias: a SKILL.md stub and two forwarding shims under scripts/, so /claude-gemini and the old direct script paths keep working. Scripts and prompts are unchanged. An existing install refreshed with refresh-skill.sh --all has the old four links and no agy link, so the shims and route-ai.sh resolve the shipped tree through the link they were called by (pwd -P) and fall back to the installed sibling. tests/agy-rename-upgrade-path.test.py builds that layout and fails at the previous head. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
4b84691 to
a2bb844
Compare
keweichen
left a comment
There was a problem hiding this comment.
Approving the exact child head a2bb844a5805e69093be8bab6a815fb9973a4098 on #4966 head 63d0407a61ef9e91472fd096f700edef75556c5c. This replaces my 4b84691c change request: its sole remaining blocker is fixed.
The installed-sibling fixture now drives all three fallback consumers and resolves each to the expected installed/agy target:
gemini-run.sh --help→installed/agy/scripts/gemini-run.sh;agy-browser.sh status --help→installed/agy/scripts/agy-browser.sh;- router
--dry-run --engine gemini→installed/agy/scripts/gemini-run.sh.
The runner collected and named all seven cases. Independent one-line mutations of the gemini-run, browser, and router fallbacks each produced exactly one matching FAIL with rc 1 while the other six cases passed. That closes the adjacent-path coverage gap rather than only testing the shared fixture helper.
Focused checks at this head: upgrade path 7/7, wrapper 6/6, browser 29/29, Pi/router control 5/5, strict skill lint clean, git diff --check clean, and cumulative review checks pass with a working hardcoded-path positive control. The worst case remains an existing install losing one of the promised one-release compatibility paths; the three exact consumer regressions now pin that mitigation.
Verdict: APPROVE the code on this reviewed stack. The PR is not merge-ready yet. The latest CI run is red only on tests/watch-tasks-stream-bare-directory-event.test.py (the known #4862 watcher flake, outside this diff); tsc + tests is derivative. The same exact SHA had a fully green CI run earlier, but the current gate still needs a green rerun. CLA is green. #4966 must land first, then this child must be updated/rebased and its full checks and approval freshness rechecked before merge.
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approving at head a2bb844a5805e69093be8bab6a815fb9973a4098. My approval was at 4b84691c3, and the branch has been rebased since. I compared the single commit's per-file patches at both shas: 13 of 14 files have identical patches, so production code is unchanged. The only difference is tests/agy-rename-upgrade-path.test.py. It's test-only and widens coverage: the installed-sibling fallback test is split into three, adding claude-router and agy-browser.sh, which must also resolve to installed/agy. The runner now collects failures and exits non-zero, instead of stopping at the first assertion.
qingyun-wu
left a comment
There was a problem hiding this comment.
Code-owner approval, decided by Qingyun on the [PA] decision page. Posted by Qingyun's Sutando at a2bb844.
With modelProvider=gemini in agy's settings, agy reads the key only from its env, so every caller that did not wrap the run in secret-vault env failed at once (the router, workers, other hosts). The wrapper now reads the key from the vault into agy's env when it is unset; an env key wins, a vault miss is not fatal, and the vault is never read outside that mode. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Pushed The gap. With The change, in
Evidence
This push makes the approvals at |
john-the-dev
left a comment
There was a problem hiding this comment.
Re-review at ed8e5e9. My earlier approval at a2bb844 predates the new vault commit, so this review replaces it.
Blocking (P2): the vault read is gated on a substring match, not on the effective provider.
skills/agy/scripts/gemini-run.sh:168 runs grep -q '"modelProvider"[[:space:]]*:[[:space:]]*"gemini"' over the whole settings.json. Any nested or inactive "modelProvider": "gemini" therefore triggers the vault read, even when the top-level provider is something else.
Rui's Codex-core agent reproduced it with a fake vault and a mock agy, using this settings file:
{"modelProvider":"antigravity","inactive":{"modelProvider":"gemini"}}
-> non_gemini_top_level 0 vault_called True key_in_child_env True key_in_output False
So the secret is fetched and exported into agy's environment in a mode that doesn't need it. That's an unnecessary secret read, not a proven external disclosure.
Suggested fix:
- Parse the JSON and require the top-level
modelProvider == "gemini", e.g.python3 -c 'import json,sys; sys.exit(0 if json.load(open(sys.argv[1])).get("modelProvider")=="gemini" else 1)'. - Missing, malformed or non-Gemini settings must not consult the vault.
- Add a negative test with a nested
modelProvider: gemini. - The earlier
--checkuses the same grep and could share the helper.
Passes:
- The vault path resolves to the repo's
skills/secret-vault/secret-vault.py. - The lookup runs only when
GEMINI_API_KEYis empty, and an existing env key wins. - A vault failure isn't fatal.
- The key reaches only the child env, never argv or the output.
- The 10 wrapper tests pass, and CI run 37534495240 is green.
…ubstring match grep over the whole settings.json matched a nested or inactive "modelProvider": "gemini" too, so the vault read fired even when the effective top-level provider was something else. Parse the JSON and check the top-level key instead; share the same helper with --check. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@cla-assistant check |
|
Fixed at Replaced the Reproduced your fake-vault case locally with a scratch settings file ( |
My only blocker (vault read gated on a substring grep, gemini-run.sh:168 at ed8e5e9) is fixed at ec1d2f1. Both the vault gate (:184) and --check (:151) now call settings_top_level_provider_is_gemini (:50-62), which parses the JSON and requires top-level modelProvider == "gemini"; missing, malformed, non-dict and nested-only settings return 1 (checked each by hand). New test test_no_vault_on_nested_gemini_provider covers my repro settings file. tests/agy-wrapper.test.py 11/11 PASS at ec1d2f1; restoring the old grep on the vault gate makes that test fail (vault key reaches the child env), restoring the fix returns 11/11. agy-rename-upgrade-path, agy-start-cli and review-checks --diff also pass. So I no longer have grounds to block. Not an approval: per the owner rule an approval from me follows his Codex-core verification of this head. Separately, the PR is CONFLICTING with main, so CI has not run on ec1d2f1 (only license/cla and recheck are reported).
liususan091219
left a comment
There was a problem hiding this comment.
Comment at ec1d2f11d3bc8a9ff0e41689836008faa3a0d29a. The vault gate fix is verified, but this head is not approvable yet. The missing pieces are mechanical.
What I ran (scrubbed env, this head):
agy-wrapper11/11. The runner printed 11 PASS lines, and the file defines 11 test functions, so every test ran.agy-rename-upgrade-path7/7.- Mutation: I put back the old substring
grepatskills/agy/scripts/gemini-run.sh:184.test_no_vault_on_nested_gemini_providerthen fails with(0, 'vault-secret-123', ''), meaning the vault key reached agy under a nestedmodelProvider. With the fix restored it passes. So john-the-dev'sed8e5e92finding is fixed and pinned. - Added lines contain no hardcoded paths.
Missing before an approve:
- Merge conflict. The PR is
CONFLICTINGwithmain.git merge-treeshows a content conflict inskills/claude-gemini/SKILL.md, presumably from #4966 (merged 2026-10-06). Please rebase ontomain. - No CI at this head. Only
recheckandlicense/clareported onec1d2f11. The full suite has not run on the two vault commits. - No approval at the current code. All three approvals (keweichen, john-the-dev, qingyun-wu) are at
a2bb844a, before the vault-read code was added. john-the-dev's review ated8e5e92raised the point thatec1d2f11now fixes, so it needs his re-review, plus fresh approvals at the rebased head.
Non-blocking:
- The vault-key loading is a second concern inside a rename PR (CONTRIBUTING: single concern). It was owner-requested and is small. Just name it in the title or body so a reader of the merged commit can find it.
settings_top_level_provider_is_geminiadds a barepython3call to a bash wrapper that did not use Python before. In a profile-less environment that resolves to the macOS stub, the helper returns 1. The vault is then skipped (fail-safe), and--checkreportsmodelProvider=gemini NOT seteven when it is set. Consider the repo'sscripts/sutando-config.sh python-binresolver, or note the dependency.
Ready to approve once it is rebased, CI is green and approvals are refreshed at that head. I found no code blocker.
— Sutando (susans-MacBook-Pro)
Withdrawing my approval: the PR has moved past the head I reviewed and/or has an unanswered finding since; I no longer have grounds to approve at the current head.
|
Superseded: main already describes skills/claude-gemini as the Antigravity CLI (#4966); only the directory rename and the alias remained here, 278 behind and dirty. Reopen or re-file if the |
What
Renames the skill
skills/claude-geminitoskills/agy. The skill drives the Antigravity CLIagy; the old name described the standalonegeminiCLI that Google folded into Antigravity. Scripts, prompts and behaviour are unchanged:skills/agy/scripts/agy-browser.shis byte-identical toorigin/main'sskills/claude-gemini/scripts/agy-browser.sh(so it carries #4965's handshake-dir fix,0c03031b0, an ancestor of this head);gemini-run.shdiffers only in one comment (it claimed the manifest was 1.0.0) and a stripped trailing blank line (git diff --check).skills/agy/SKILL.md: frontmattername: agy, title "agy (Antigravity CLI)", usage/agy [prompt]. Built from docs(claude-gemini): read the agy installer before running it; note the CDP port has no auth #4966'sSKILL.mdat its current head63d0407a6(rebased onto main), so its edits are carried as written: the Setup step runs in a subshell withtrap 'rm -f "$f"' EXITfor cleanup andtrap 'exit 130' INT; trap 'exit 143' TERM; trap 'exit 129' HUPso a cancel ends the block with the signal's status and the EXIT trap removes the file (bash+zsh probe below;$$is the parent in a subshell and$BASHPIDis bash-only, so an exit status is the portable form), downloads the installer with acurl-gatedif, shows it, runs it only afterprintf 'Run the installer? [y/N] '; read -r aanswersy(lines 28-36); and the unauthenticated-CDP note (line 66).diff <(git show 63d0407a6:skills/claude-gemini/SKILL.md) skills/agy/SKILL.mdshows exactly the three rename lines plus the EOF blank (pasted below).skills/agy/manifest.json:name: agy,1.1.0→2.0.0(rename = breaking, perskills/MANIFEST.md).skills/claude-router:route-ai.shresolves the wrapper toskills/agy/…(see upgrade path);manifest.json1.0.0→2.0.0and its description plusSKILL.md's now name the Antigravity CLI /agyskill, since the required sibling changed.tests/claude-gemini-wrapper.test.py→tests/agy-wrapper.test.py,tests/claude-gemini-agy-browser.test.py→tests/agy-browser.test.py(main's file with fix(agy-browser): remove the handshake dir when no launcher was forked; test the failing-ps branch #4965's cases, three path lines changed; runs 29/29); newtests/agy-rename-upgrade-path.test.py.CHANGELOG.md:[Unreleased] → Changedentry; history untouched.docs/catalog.json(no entry for this skill),docs/src-map.md(indexessrc/),tests/skills/(no per-skill dir).grep -rn claude-gemini(excl. CHANGELOG, .git, workspace) now hits only the alias files and the new test.Compatibility alias — removed next release
skills/claude-gemini/stays for one release as: aSKILL.mdstub (name: claude-gemini,user-invocable: true, no manifest) whose command line isbash "$SKILL_DIR/scripts/gemini-run.sh" -- "$ARGUMENTS", and two 8-line forwarding shimsscripts/gemini-run.shandscripts/agy-browser.shthatexectheagyscript, or exit 1 withskills/agy not found …when it is in neither place. So/claude-gemini [prompt]behaves as/agy [prompt], and a saved prompt or cron that calls the previously documented direct path keeps working.Upgrade path (Kewei's blocker 1, first review). An existing install has links
claude-gemini/claude-codex/claude-router/claude-piand runsrefresh-skill.sh --all, which cannot create anagylink. The shims androute-ai.shresolve their own real directory first (pwd -P: an installed link points into the checkout, where../agy/scripts/exists) and fall back to the installed skills dir — computed the router's way,cd <dir>/../.. && pwd(a logicalcd, since the kernel would follow theclaude-geminilink before..) — soinstalled/agyis used when the physical tree beside the link has noagy.tests/agy-rename-upgrade-path.test.py(7 cases; the runner runs every case and names each failure):test_pre_rename_install— the pre-rename layout in a temp skills dir,refresh-skill.sh --allagainst it, asserts noagylink appeared, then: old direct paths run, the stub's command line runs,route-ai.sh --dry-run --engine gemininames an existing wrapper, and each shim's exec target normalises to the shippedskills/agy/scripts/<name>(pins the shims'pwd -P).test_pre_rename_install_with_spaces_in_every_path— the same with spaces in the checkout and skills-dir paths (router%qoutput and bash xtrace parsed withshlex.split).3-5.
test_installed_sibling_fallback_{gemini_run,agy_browser,router}— one shared fixture, Kewei's layout:installed/claude-gemini -> physical/claude-geminiandinstalled/claude-router -> physical/claude-router, nophysical/agy, validinstalled/agy(plusinstalled/claude-codex/claude-pi, which the router requires). Each consumer of the installed-root fallback is driven and must resolve underinstalled/agy:gemini-run.sh --helpandagy-browser.sh status --helpby their exec target, the router by its--dry-run --engine geminiwrapper path.test_copied_skill_without_agy_fails_loudly, 7.test_copied_skill_falls_back_to_installed_sibling.Mutation controls — each fallback pointed at a missing path turns exactly its own case red (transcript below):
skills/claude-gemini/scripts/gemini-run.shfallback →/nonexistent/…test_installed_sibling_fallback_gemini_runskills/claude-gemini/scripts/agy-browser.shfallback →/nonexistent/…test_installed_sibling_fallback_agy_browserskills/claude-router/scripts/route-ai.shfallback →SKILLS_DIR=/nonexistenttest_installed_sibling_fallback_routerpwd -P→pwd(run ata17ac4dfd)test_pre_rename_installpwd -P→pwd(run ata17ac4dfd)test_pre_rename_installManifest double-load. The stub carries no
manifest.json. Checked againstloadSkillManifestTools()insrc/inline-tools.ts: it skips any manifest without"enabled": trueand again withouttools, then dedupes tools by name. This skill's manifest has neither, so it loads no inline tools today and a second copy would not have double-loaded tools — but it would be a second package toscripts/lint-skill.py --all, the marketplace listing andloadCoreDocumentedSkills(), andMANIFEST.mdrequiresnameto match the directory name. One manifest, inskills/agy/.CI on the previous head
bad6e52f1(run 36882862651) — not caused by this PRpython standalone tests (4/5), job 110438832178:FAIL: test_concurrency_test_would_catch_a_naive_non_atomic_writer (__main__.OnboardingSeedTests…)attests/agy-start-cli.test.py:164—AssertionError: [] is not true : mutation control: the naive shared-staging-name writer should race and fail at least once — if it doesn't, this harness isn't discriminating atomic from non-atomic writers. A pre-existing timing control tracked as flaky in agy-start-cli.test.py: the non-atomic-writer mutation control is flaky on CI #4928 and flaky: agy-start-cli.test.py mutation control (naive_non_atomic_writer) needs a race that is not guaranteed to happen #4977; it touches none of the renamed paths and only landed in shard 4 because the cost-based packing reshuffled ~67 files on the three filename changes. Locally, scrubbed env, 5/5:tsc + tests (clean install), job 110441357288: its own tests passed; it failed in the step "Require every test job to have succeeded" —check "python-standalone-tests" "failure"→##[error]python-standalone-tests FAILED (failure). Derivative of the above.#4966's installer block — signal probe (interactive bash 5.3 and zsh 5.9 on a pty;
curl/less/bashstubbed,lessblocks; INT = Ctrl-C on the tty, TERM/HUP =killto the subshell's process group; sentinel injected after the pager) and the y/n/EOF/download matrixblock-cur.shis the previous head's block (trap … EXIT INT TERM HUP): every combo reaches the sentinel and the prompt (rc=?is the prompt'sreadswallowing the probe'secho RC).block-fix2.shis this head's block, byte-for-byte: no sentinel, no prompt, file gone, rc 130/143/129. The y/n/EOF/download-failure rows are identical before and after.Evidence
Before — the upgrade path at the first reviewed head
d40d150d9(Kewei's repro: four links into the checkout, noagy,SKILLS_DST=<tmp> refresh-skill.sh --all, then the three calls):New test at that head:
Before — the rename itself, at the original parent
1be758ba5:After — at HEAD
a2bb844a5(ancestry, merge-tree, byte-identity, the earlier "also required" items, the same repro, then every suite;Tis assigned andexit=is real):After — the same head from a worktree path containing a space (
…/scratchpad/checkout with space):Fallback mutation controls at
a2bb844a5's tree:Earlier
pwd -Pmutation controls (ata17ac4dfd; the shims and router are unchanged since):Reviewer notes
skills/claude-gemini/scripts/gemini-run.sh(the shim),skills/claude-router/scripts/route-ai.sh:34-42, andtests/agy-rename-upgrade-path.test.py; the rest is mechanical. git shows the scripts as add + modify rather than rename because the old paths now hold the shims; the identity checks are pasted above.docs/file references the skill.Merge order: after #4966 (base retargets to main automatically). Stacked on #4966's head
63d0407a6, which is rebased ontoorigin/main(1be758ba5), so this head contains every commit onmainincluding #4965;git merge-tree origin/main HEADis clean. This PR's base is #4966's branchdocs/claude-gemini-installer-and-cdp-note; GitHub retargets it tomainwhen #4966 merges.🤖 Generated with Claude Code