fix(lib): resolve root causes of login prompt in claude and TOS/theme onboarding in agy
This commit is contained in:
@@ -0,0 +1,122 @@
|
||||
# Code Review: `.agents/skills/lib.sh` — claude `/login` + agy TOS/theme seeding fix (cp -a migration)
|
||||
|
||||
**Job ID**: aab82a5b
|
||||
**Reviewer**: cline
|
||||
**Diff reviewed**: `f2e23c1..ea863c0` (working tree, `git diff HEAD`)
|
||||
|
||||
## Scope
|
||||
|
||||
This diff (`git diff HEAD -- .agents/skills/lib.sh`) modifies only the `provision_isolation()` function's `claude)` and `agy)` case arms. It is the next iteration of the agy TOS-seeding fix (commit `6692c27`) and adds a parallel fix for the claude `/login` prompt. Two themes of change:
|
||||
|
||||
1. **claude arm** — add seeding for `~/.claude/session-env`, `sessions`, `cache`; make `.credentials.json` symlink guarded by an existence check; switch `settings.json` from a symlink to an idempotent `cp -a` copy (so the isolated agent can write its own settings without mutating the host file).
|
||||
2. **agy arm** — extend the `antigravity-cli` file list (`conversation_summaries.db`, `jetski_state.pbtxt`); add `~/.gemini/antigravity-ide` (cp -a); add `com.google.antigravity-ide.plist` + `com.google.GeminiMacOS.launcher.plist` to the plist loop; switch all Preferences plists, `Application Support/Antigravity`, and the new `Application Support/Antigravity IDE` + `com.google.GeminiMacOS` from symlinks to idempotent `cp -a` copies; hoist `mkdir -p "$root/Library/Application Support"` before the conditional blocks; add the Linux XDG `cp -a` migration; fix the carried-forward `seeded="$seeded,..."` guard inconsistency to `${seeded:+$seeded,}` everywhere.
|
||||
|
||||
## "No other files changed" check
|
||||
|
||||
`git status --short`:
|
||||
- `M .agents/skills/lib.sh` — the only tracked-file modification.
|
||||
- `?? .DS_Store` — macOS Finder metadata, untracked, not a code change (pre-existing, not created by this diff).
|
||||
- `?? .agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job.<pid>_<n>.tmp` — untracked, transient atomic-write temp copy of the job-runner orchestrator script (same artifact observed in prior review passes), not a code change.
|
||||
|
||||
**Confirmed: no files other than `.agents/skills/lib.sh` contain reviewable changes.**
|
||||
|
||||
## Lint / Syntax
|
||||
|
||||
- `bash -n .agents/skills/lib.sh` → **SYNTAX OK**.
|
||||
- `shellcheck` not installed; manual review found no quoting, unbound-variable, or word-splitting issues in the new lines.
|
||||
- The shebang is `#!/usr/bin/env bash`; no new bash-specific constructs beyond what the file already uses.
|
||||
|
||||
## Hunk 1 — `claude` arm
|
||||
|
||||
### Changes
|
||||
- `.credentials.json`: was unconditional `ln -sfn` (would fail/produce dangling link if source absent); now guarded `if [ -e ... ]` then `ln -sfn`. Correctness improvement.
|
||||
- `settings.json`: was `ln -sfn` (host-mutating risk if isolated claude writes settings); now `if [ ! -e "$root/settings.json" ]; then cp -a ...; fi` — idempotent physical copy. The isolated agent can now write its own settings without mutating the host's `~/.claude/settings.json`.
|
||||
- `plugins`, `session-env`, `sessions`, `cache`: new guarded `ln -sfn` symlinks. All use the `${seeded:+$seeded,}` guard consistently.
|
||||
- The two pre-existing `seeded="$seeded,..."` lines (old `settings.json`, `plugins`) are converted to `${seeded:+$seeded,}` — this **fixes the carried-forward cosmetic nit** from prior reviews (leading-comma risk in the seeded log string).
|
||||
|
||||
### Correctness & scoping
|
||||
- Correctly scoped to the `claude)` arm. `claude` uses the `claude_config_dir` isolation lever (`isolation_env_prefix` → `CLAUDE_CONFIG_DIR=<root>`), so claude reads config from `$root` directly — the new `session-env`, `sessions`, `cache` symlinks at `$root/...` are the right target.
|
||||
- All new source paths guarded with `[ -d ... ]` / `[ -e ... ]` before linking; `settings.json` copy guarded with `[ ! -e "$root/settings.json" ]` for idempotency.
|
||||
|
||||
### Live path verification (this machine = macOS Darwin)
|
||||
| Path | Exists? |
|
||||
|---|---|
|
||||
| `~/.claude.json` | ✅ |
|
||||
| `~/.claude/.credentials.json` | absent (so the new guard correctly skips it — old code would have created a dangling symlink) |
|
||||
| `~/.claude/settings.json` | ✅ |
|
||||
| `~/.claude/plugins` | ✅ |
|
||||
| `~/.claude/session-env` | ✅ |
|
||||
| `~/.claude/sessions` | ✅ |
|
||||
| `~/.claude/cache` | ✅ |
|
||||
|
||||
### Functional smoke test
|
||||
Ran `provision_isolation claude <tmp_root>` against live state. Result:
|
||||
- rc=0.
|
||||
- Seeded list: `.claude.json,settings.json,plugins,session-env,sessions,cache` (note: no `.credentials.json` because source is absent — guard works; no leading comma).
|
||||
- `$root/settings.json` is a **regular file** (`-rw-------`, cp -a copy), not a symlink — as intended.
|
||||
- `$root/.claude.json`, `plugins`, `session-env`, `sessions`, `cache` are correct symlinks.
|
||||
- Idempotency: re-running against the same root returns rc=0; `settings.json` is not re-copied (guard works).
|
||||
|
||||
### Existing test impact
|
||||
The only `provision_isolation` test, `test_comp_create_isolation_folder_setup` (tests/test_tier2_component.py:99-116), asserts `cred_sym.is_symlink()` for `.credentials.json`. The test's `mam_sandbox` fixture (conftest.py:33) sets `HOME=tmp_path`, and the test creates `tmp_path/.claude/.credentials.json`, so the new `if [ -e ... ]` guard passes and the symlink is still created — the test's `is_symlink()` assertion still holds. The test does not assert on `settings.json`, so the symlink→cp change is invisible to it. **No test breakage.** (Note: the test suite cannot be executed here — pytest is not installed and conftest hardcodes a Linux `src_skills` path `/home/godopu16/...` — but the logic analysis confirms no regression.)
|
||||
|
||||
## Hunk 2 — `agy` arm
|
||||
|
||||
### Changes
|
||||
- `antigravity-cli` file list: added `conversation_summaries.db`, `jetski_state.pbtxt` (both exist on this machine). Additive, guarded by `[ -e ... ]`.
|
||||
- `~/.gemini/antigravity-ide`: new block, `cp -a` (not symlink) with `[ ! -d ... ]` idempotency guard. Physical copy so the isolated IDE can write its own state.
|
||||
- Plist loop: added `com.google.antigravity-ide.plist`, `com.google.GeminiMacOS.launcher.plist`; switched all plists from `ln -sfn` to idempotent `cp -a`; converted `seeded="$seeded,..."` → `${seeded:+$seeded,}`.
|
||||
- `mkdir -p "$root/Library/Application Support"` hoisted before the conditional blocks (was inside each `if`), so the new `Antigravity IDE` and `com.google.GeminiMacOS` blocks can copy without each repeating the mkdir.
|
||||
- `Application Support/Antigravity`: symlink → idempotent `cp -a`.
|
||||
- `Application Support/Antigravity IDE`: new block, `cp -a`.
|
||||
- `Application Support/com.google.GeminiMacOS`: new block, `cp -a`.
|
||||
- `Group Containers/group.com.google.gemini`: kept as symlink (shared live IPC container), only the `seeded` guard was fixed to `${seeded:+$seeded,}`.
|
||||
- Linux XDG branch: all four `ln -sfn` → idempotent `cp -a` with `[ ! -d ... ]` guards; comment updated to note write isolation.
|
||||
|
||||
### Correctness & scoping
|
||||
- Correctly scoped to the `agy)` arm; `agy` uses the `home` isolation lever, so `$root/.gemini/...` and `$root/Library/...` are the right targets.
|
||||
- The `cp -a` migration is the right call for dirs the isolated agent will **write to** (TOS acceptance, theme selection, conversation history) — a symlink would funnel those writes back to the host, defeating isolation and potentially corrupting the host's Antigravity state. Keychains and Group Containers stay symlinked because those are read-only credential/IPC lookups that must stay live.
|
||||
- All new source paths guarded; all `cp -a` targets guarded with `[ ! -d/-f/-e ... ]` for idempotency.
|
||||
- The hoisted `mkdir -p "$root/Library/Application Support"` is safe — `mkdir -p` is a no-op if the dir already exists.
|
||||
|
||||
### Live path verification (this machine = macOS Darwin)
|
||||
All newly-referenced source paths exist:
|
||||
| Path | Exists? |
|
||||
|---|---|
|
||||
| `~/.gemini/antigravity-ide` | ✅ dir |
|
||||
| `~/.gemini/antigravity-cli/conversation_summaries.db` | ✅ |
|
||||
| `~/.gemini/antigravity-cli/jetski_state.pbtxt` | ✅ |
|
||||
| `~/Library/Preferences/com.google.antigravity-ide.plist` | ✅ |
|
||||
| `~/Library/Preferences/com.google.GeminiMacOS.launcher.plist` | ✅ |
|
||||
| `~/Library/Application Support/Antigravity IDE` | ✅ dir |
|
||||
| `~/Library/Application Support/com.google.GeminiMacOS` | ✅ dir |
|
||||
|
||||
### Functional smoke test
|
||||
Ran `provision_isolation agy <tmp_root>` against live state. Result:
|
||||
- rc=0.
|
||||
- Seeded list (18 entries): `.gemini/antigravity-cli/{antigravity-oauth-token,installation_id,settings.json,conversation_summaries.db,jetski_state.pbtxt}`, `.gemini/antigravity`, `.gemini/antigravity-ide`, `.gemini/config`, `Library/Keychains`, `Library/Preferences/{com.google.antigravity.plist,com.google.antigravity-ide.plist,com.google.GeminiMacOS.plist,com.google.GeminiMacOS.shareddata.plist,com.google.GeminiMacOS.launcher.plist}`, `Library/Application Support/{Antigravity,Antigravity IDE,com.google.GeminiMacOS}`, `Library/Group Containers/group.com.google.gemini`. No leading comma.
|
||||
- `$root/.gemini/antigravity-ide`, `$root/Library/Application Support/Antigravity`, `Antigravity IDE`, `com.google.GeminiMacOS` are **physical directory copies** (not symlinks) — as intended.
|
||||
- `$root/.gemini/antigravity`, `.gemini/config`, `Library/Keychains`, `Group Containers/...` remain symlinks — as intended.
|
||||
- Idempotency: re-running against the same root returns rc=0; no re-copy.
|
||||
|
||||
### `cp -a` socket warning (benign, expected)
|
||||
`~/Library/Application Support/Antigravity IDE/1.10-main.sock` is a Unix socket (live IDE IPC handle). `cp -a` **skips sockets by design**, prints `cp: ... is a socket (not copied).` to stderr, and **returns exit code 0** — verified by reproducing with a synthetic socket. The dir copy still contains every regular file/subdir. This is harmless: the socket is a transient runtime handle the isolated agy/IDE would recreate on its own; copying it would be meaningless. The function returns 0 and all real config/data is seeded. Not a defect.
|
||||
|
||||
## Regression / loss check
|
||||
- **No functionality removed.** The claude `.credentials.json` change is a strict improvement (guard prevents dangling symlinks). The `settings.json` symlink→cp and agy symlink→cp migrations are intentional behavior changes that improve write isolation — the isolated agent can now write its own TOS/theme/settings state without mutating the host.
|
||||
- The prior-review cosmetic nit (Darwin `seeded="$seeded,..."` missing the `${seeded:+$seeded,}` guard) is **fully resolved** — every `seeded` assignment in both arms now uses the guard.
|
||||
- The `cp -a` idempotency guards (`[ ! -e/-d/-f "$root/..." ]`) make re-provisioning to the same root safe (verified: re-run returns rc=0, no re-copy, no error).
|
||||
- No imports/variables orphaned by these changes.
|
||||
- **Pre-existing test-coverage gap (not introduced by this diff):** no test exercises the `agy` arm of `provision_isolation`, and the claude-arm test does not cover the new `session-env`/`sessions`/`cache` symlinks or the `settings.json` cp behavior. Flagging for awareness, not a blocker — adding agy-arm coverage would be a worthwhile follow-up but is out of scope for this review.
|
||||
|
||||
## Design assessment
|
||||
This is the right level of change — a targeted bug fix, not a redesign:
|
||||
- The root-cause analysis (claude prompts `/login` because session-env/sessions/cache weren't seeded; agy prompts TOS/theme because writable state dirs were symlinks back to host) is addressed at the correct layer (the seeding function), not by patching around the prompts downstream.
|
||||
- The symlink-vs-copy distinction is applied correctly: read-only credential/IPC lookups (Keychains, Group Containers, `.gemini/antigravity`, `.gemini/config`) stay symlinked; writable state dirs (settings, Application Support, antigravity-ide, XDG dirs, plists) become copies. This matches the isolation intent.
|
||||
- No re-planning/rework needed.
|
||||
|
||||
## Verdict
|
||||
|
||||
Both hunks are correctly scoped (only `.agents/skills/lib.sh`, only `provision_isolation`), syntactically valid (`bash -n` clean), and functionally verified against live machine state with smoke tests (rc=0, correct symlink/copy layout, idempotent re-runs). The diff fixes the carried-forward `seeded` guard nit, improves the claude `.credentials.json` guard, and migrates writable state dirs from host-mutating symlinks to isolated `cp -a` copies — directly addressing the stated root causes for both the claude `/login` and agy TOS/theme prompts. The only stderr noise (`cp -a` socket-skip warning) is benign, expected `cp` behavior with exit code 0. No regressions, no loss, no test breakage. No escalation needed.
|
||||
|
||||
[VERDICT: PASS]
|
||||
Reference in New Issue
Block a user