10 KiB
Code Review: .agents/skills/lib.sh — agy TOS-seeding fix + send_keys_safe cline/claude paste-check skip
Job ID: 5157a4a7
Reviewer: cline
Diff reviewed: 3846d99..f2e23c1 (working tree, git diff HEAD)
Scope
The diff (git diff HEAD -- .agents/skills/lib.sh, +66/-12) contains two hunks in .agents/skills/lib.sh:
- Hunk 1 —
provision_isolation()agy-arm: adds seeding for~/.gemini/antigravity,~/.gemini/config, macOSLibrary/Preferencesplists,Library/Application Support/Antigravity,Library/Group Containers/group.com.google.gemini, and a Linux/Unixelsebranch seeding XDG config/data dirs. This is the change directly described in the task goal. - Hunk 2 —
send_keys_safe(): skips the strict paste-visibility check (step 3) for sessions whose name matchesclineorclaude, while still running the step-4 submission-verification retry loop.
The working-tree diff matches the diff quoted in the brief byte-for-byte.
"No other files changed" check
git status --short:
M .agents/skills/lib.sh— the only tracked-file modification.?? .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.shellcheckis not installed in this environment; manual review of the new lines found no quoting, unbound-variable, or word-splitting issues.
Hunk 1 — provision_isolation agy TOS seeding
Correctness & scoping
- Correctly scoped to the
agy)case arm ofprovision_isolation.agyuses thehomeisolation lever (isolation_lever→home,isolation_env_prefix→HOME=<root>), so seeding into$root/.gemini/...and$root/Library/...is the right target for a redirectedHOME. - All source paths are guarded with existence checks (
[ -d ... ]/[ -f ... ]) beforeln -sfn, matching the established pattern in the surrounding code, so the function is a no-op for paths that don't exist on a given machine. mkdir -pis called for each new parent dir ($root/Library/Preferences,$root/Library/Application Support,$root/Library/Group Containers,$root/.config,$root/.local/share) before creating the symlink, soln -sfnnever fails on a missing parent.- The Linux
elsebranch correctly resolvesXDG_CONFIG_HOME/XDG_DATA_HOMEwith${VAR:-$HOME/...}defaults and tries bothAntigravityandantigravitycasings viaelif, mirroring the case-sensitivity reality of XDG dirs across distros. - The two new
~/.gemini/antigravityand~/.gemini/configblocks are placed after the existingantigravity-clifile loop and before theunameDarwin/Linux branch, which is the correct location (they apply on both platforms; theunamebranch is platform-specific).
Live path verification (this machine = macOS Darwin)
All newly-referenced source paths were checked against the real $HOME:
| Path | Exists? |
|---|---|
~/.gemini/antigravity |
✅ dir |
~/.gemini/config |
✅ dir |
~/Library/Keychains |
✅ dir (pre-existing seed target) |
~/Library/Preferences/com.google.antigravity.plist |
✅ file |
~/Library/Preferences/com.google.GeminiMacOS.plist |
✅ file |
~/Library/Preferences/com.google.GeminiMacOS.shareddata.plist |
✅ file |
~/Library/Application Support/Antigravity |
✅ dir |
~/Library/Group Containers/group.com.google.gemini |
✅ dir |
Functional smoke test
Ran provision_isolation agy <tmp_root> against the live machine state. Result:
- Return code 0.
- Seeded list printed:
.gemini/antigravity-cli/antigravity-oauth-token,.gemini/antigravity-cli/installation_id,.gemini/antigravity-cli/settings.json,.gemini/antigravity,.gemini/config,Library/Keychains,Library/Preferences/com.google.antigravity.plist,Library/Preferences/com.google.GeminiMacOS.plist,Library/Preferences/com.google.GeminiMacOS.shareddata.plist,Library/Application Support/Antigravity,Library/Group Containers/group.com.google.gemini - Every entry under
<tmp_root>is a correct symlink (ls -laR) pointing at the real source path; no dangling links, no leading comma in the output (because earlier.gemini/*segments fire first on this machine).
Cosmetic nit (carried forward, inert)
Three Darwin-branch lines build seeded as seeded="$seeded,<path>" (lines 1240, 1246, 1251) without the ${seeded:+$seeded,} guard used everywhere else in this function (including the two .gemini/* blocks added in this same diff, just above). If none of the earlier seed steps fired (e.g. a fresh machine with no ~/.gemini/* files and no Keychains dir, but an existing Antigravity Preferences plist), seeded would start with a leading comma (e.g. ,Library/Preferences/com.google.antigravity.plist).
- Impact: the sole consumer,
create_session.sh:339, does[x for x in os.environ.get('ISOLATION_SEEDED', '').split(',') if x], which filters falsy split segments — so a leading empty element is silently dropped. No functional bug today. Style consistency nit only; does not block.
local in case branch (bash semantics)
The Linux else branch declares local xdg_config=... / local xdg_data=... inside a case arm. In bash, local is dynamically scoped and valid anywhere inside a function body regardless of case/if nesting, so this is well-formed (and bash -n confirms). No issue.
Hunk 2 — send_keys_safe() cline/claude paste-check skip
local was_popup=0
if [[ "$sess" =~ "cline" ]] || [[ "$sess" =~ "claude" ]]; then
# Skip strict paste check due to scrollout false-positives, proceed to C-m loop
true
else
sleep 0.5
local pane_content
pane_content=$(_pane_capture "$sess")
...paste-visibility checks (return 3 on miss)...
fi
Scope observation
The stated task goal is the agy TOS-seeding fix (Hunk 1). Hunk 2 is a separate change to the paste-submission verification logic for cline/claude sessions and is unrelated to agy onboarding. However, the brief explicitly asks to review the cumulative git diff, so it is in scope for this review. It is not a blocker (see analysis below), but it is noted that this hunk is outside the narrow task-goal description.
Analysis
- Consistent with existing convention: three lines above (unchanged, pre-existing context), the function already special-cases
if [[ "$sess" =~ "agy" ]]to skip all verification andreturn 0unconditionally. The newcline/claudebranch is less aggressive — it only skips step 3 (paste-visibility check) and still runs the step-4 submission-verification retry loop (marker-left-tail / pane-changed / spinner-token checks) below. It is more conservative than the precedent it sits next to, not a new pattern. - Session-name matching is safe under this project's naming convention: session names embed the agent name as a suffix (e.g.
...-creator-claude,...-creator-cline,...-creator-agy), matching the pattern already relied upon by the pre-existingagycheck, so substring matching oncline/claudeis not expected to produce false hits from unrelated workspace/repo names. =~with quoted RHS:[[ "$sess" =~ "cline" ]]— quoting the RHS of=~makes bash treat it as a literal substring match rather than a regex (shellcheck SC2076-class nit;[[ "$sess" == *cline* ]]would be the idiomatic form). This exactly mirrors the pre-existing, unchangedagycheck one line above, so it is a style-consistency choice, not a regression.- Variable scoping: splitting the old
local pane_content was_popup=0intolocal was_popup=0(outer) andlocal pane_content(inner, else-only) is correct —was_popupis referenced unconditionally later in the retry loop (lines 1653, 1655),pane_contentonly inside the branch that declares it. No unbound-variable risk. - Trade-off (named explicitly): skipping the paste-visibility check for claude/cline means a genuine paste failure (not just a scrollback false-positive) for those two agents is no longer caught at step 3 (
return 3); it now depends entirely on the step-4 retry-loop heuristics (spinner tokens, marker leaving the tail, pane-content diff) to detect submission. This is a reasonable, bounded trade-off given the stated motivation (documented false positives breaking real pastes for these TUIs), and it downgrades detection rather than removing it. It would benefit from a live smoke test (send a real multi-line prompt to aclaude/cline-suffixed session and confirm it submits) which cannot be performed in this non-interactive review environment — but nothing here indicates the fix is wrong or requires a redesign. bash -npasses; no unbound-variable or quoting issues found in this hunk.
Regression / loss check
- No existing functionality removed: the
agyearly-return shortcut (line 1619-1622) is unchanged; theelse(non-cline/claude/agy) path retains the original strict paste-visibility check verbatim. - The two
.gemini/*blocks added in Hunk 1 are additive and guarded by existence checks; they cannot break the priorantigravity-clifile loop above them. - No imports/variables orphaned by these changes.
- No tests exist that exercise the agy arm of
provision_isolation(the onlyprovision_isolationtest,test_comp_create_isolation_folder_setup, exercises theclaudearm), so there is no test regression to report — and no new test was added for the agy arm. This is a pre-existing test-coverage gap, not introduced by this diff; flagging for awareness, not as a blocker.
Verdict
Both hunks are correctly scoped (only .agents/skills/lib.sh, only their respective functions), syntactically valid (bash -n clean), and internally consistent with existing patterns in the same file. Hunk 1 is the agy TOS/onboarding seeding fix directly matching the task goal; it was functionally smoke-tested against live machine state and produces correct symlinks and a well-formed seeded list. Hunk 2 is a targeted flakiness fix for cline/claude paste submission that follows the file's existing per-agent-shortcut convention and is more conservative than the precedent it sits next to; its only real risk (masked true paste failures for claude/cline) is a bounded, intentional trade-off that would ideally get a live smoke test, but nothing here indicates the fix is wrong or requires a redesign. The two cosmetic nits (Darwin seeded guard inconsistency; =~ quoted-RHS style) are inert and do not block.
[VERDICT: PASS]