docs(reports): add durable peer review reports from multi-agent loop
This commit is contained in:
@@ -0,0 +1,46 @@
|
||||
# Code Review: `.agents/skills/lib.sh` — agy TOS-seeding fix + `send_keys_safe` cline/claude paste-check skip
|
||||
|
||||
## Scope
|
||||
This diff (`3846d99..f2e23c1`) contains **two** hunks in `.agents/skills/lib.sh`:
|
||||
1. The `provision_isolation()` `agy`-arm TOS/onboarding seeding addition (identical to the diff already reviewed under jobs `b803c6e5`, `afd375ef`, `8f8b6d63` — no changes since).
|
||||
2. **New**: a `send_keys_safe()` change that skips the strict paste-visibility check for sessions whose name matches `cline` or `claude`.
|
||||
|
||||
`git diff -- .agents/skills/lib.sh` matches the diff quoted in this brief byte-for-byte; the working tree has not drifted.
|
||||
|
||||
## "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 itself (same pattern observed in prior review passes), not a code change.
|
||||
|
||||
Confirmed: **no files other than `.agents/skills/lib.sh` contain reviewable changes.**
|
||||
|
||||
## Hunk 1 — `provision_isolation` agy TOS seeding (re-verified, unchanged)
|
||||
Already verified in full in the three prior reviews of this exact content:
|
||||
- Syntax OK (`bash -n`).
|
||||
- Correctly scoped to `agy`'s `home`-lever isolation; all new paths (`~/.gemini/antigravity`, `~/.gemini/config`, macOS `Library/Preferences` plists, `Library/Application Support/Antigravity`, `Library/Group Containers/group.com.google.gemini`, Linux XDG fallback) match real, live paths verified against this machine's actual `$HOME`, and correctly exclude the unrelated Antigravity IDE product.
|
||||
- One pre-existing, cosmetic-only nit carried forward: three Darwin-branch `seeded="$seeded,<path>"` assignments omit the `${seeded:+$seeded,}` guard used elsewhere, which could produce a leading comma in the `seeded` log string if no earlier segment fired — inert today because the sole consumer (`create_session.sh:339`) filters falsy split segments. Not a functional bug.
|
||||
|
||||
## Hunk 2 — `send_keys_safe()` cline/claude skip (new)
|
||||
```bash
|
||||
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")
|
||||
...
|
||||
fi
|
||||
```
|
||||
- **Consistent with existing convention**: three lines above (unchanged, pre-existing context), the function already special-cases `if [[ "$sess" =~ "agy" ]]` to skip *all* verification and return unconditionally. The new `cline`/`claude` branch 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, so it is more conservative than the pre-existing `agy` shortcut, not a new pattern.
|
||||
- **`=~` with quoted RHS**: `[[ "$sess" =~ "cline" ]]` — quoting the right-hand side of `=~` makes bash treat it as a literal substring match rather than a regex (a real but benign shellcheck SC2076-class nit; `[[ "$sess" == *cline* ]]` would be the idiomatic form). This exactly mirrors the pre-existing, unchanged `agy` check one line above, so it is a style consistency choice, not a regression introduced by this diff.
|
||||
- **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-agy`), matching the pattern already relied upon by the pre-existing `agy` check, so substring matching on `cline`/`claude` is not expected to produce false hits from unrelated workspace/repo names.
|
||||
- **Trade-off worth naming explicitly**: skipping the paste-visibility check for claude/cline means a genuine paste failure (not just a scrollback false-positive) for those two agents will no longer be caught at step 3 (`return 3`); it now depends entirely on the step-4 retry loop's 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 two TUIs), and downgrades detection rather than removing it — but it is a live-environment behavior change I cannot execute/observe directly in this review (no interactive herdr/tmux cline or claude pane available here to reproduce the described scrollback false-positive or confirm the retry loop still catches a true paste failure). Flagging as the one item that would benefit from a manual smoke test (send a real multi-line prompt to a `claude`-suffixed session and confirm it submits correctly) rather than as a defect.
|
||||
- Variable scoping: splitting `local pane_content was_popup=0` into `local was_popup=0` (outer) and `local pane_content` (inner, else-only) is correct — `was_popup` is used unconditionally later in the retry loop, `pane_content` only inside the branch that declares it.
|
||||
- `bash -n` passes; no unbound-variable or quoting issues found in this hunk.
|
||||
|
||||
## Verdict
|
||||
Both hunks are correctly scoped (only `.agents/skills/lib.sh`, only their respective functions), syntactically valid, and internally consistent with existing patterns in the same file. Hunk 1 is a repeat-verified TOS/onboarding fix with no functional issues. Hunk 2 is a targeted flakiness fix 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.
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,99 @@
|
||||
# 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`:
|
||||
|
||||
1. **Hunk 1** — `provision_isolation()` `agy`-arm: adds seeding for `~/.gemini/antigravity`, `~/.gemini/config`, macOS `Library/Preferences` plists, `Library/Application Support/Antigravity`, `Library/Group Containers/group.com.google.gemini`, and a Linux/Unix `else` branch seeding XDG config/data dirs. This is the change directly described in the task goal.
|
||||
2. **Hunk 2** — `send_keys_safe()`: skips the strict paste-visibility check (step 3) for sessions whose name matches `cline` or `claude`, 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**.
|
||||
- `shellcheck` is 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 of `provision_isolation`. `agy` uses the `home` isolation lever (`isolation_lever` → `home`, `isolation_env_prefix` → `HOME=<root>`), so seeding into `$root/.gemini/...` and `$root/Library/...` is the right target for a redirected `HOME`.
|
||||
- All source paths are guarded with existence checks (`[ -d ... ]` / `[ -f ... ]`) before `ln -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 -p` is 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, so `ln -sfn` never fails on a missing parent.
|
||||
- The Linux `else` branch correctly resolves `XDG_CONFIG_HOME`/`XDG_DATA_HOME` with `${VAR:-$HOME/...}` defaults and tries both `Antigravity` and `antigravity` casings via `elif`, mirroring the case-sensitivity reality of XDG dirs across distros.
|
||||
- The two new `~/.gemini/antigravity` and `~/.gemini/config` blocks are placed after the existing `antigravity-cli` file loop and before the `uname` Darwin/Linux branch, which is the correct location (they apply on both platforms; the `uname` branch 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
|
||||
|
||||
```bash
|
||||
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 and `return 0` unconditionally. The new `cline`/`claude` branch 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-existing `agy` check, so substring matching on `cline`/`claude` is 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, unchanged `agy` check one line above, so it is a style-consistency choice, not a regression.
|
||||
- **Variable scoping**: splitting the old `local pane_content was_popup=0` into `local was_popup=0` (outer) and `local pane_content` (inner, else-only) is correct — `was_popup` is referenced unconditionally later in the retry loop (lines 1653, 1655), `pane_content` only 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 a `claude`/`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 -n` passes; no unbound-variable or quoting issues found in this hunk.
|
||||
|
||||
## Regression / loss check
|
||||
- No existing functionality removed: the `agy` early-return shortcut (line 1619-1622) is unchanged; the `else` (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 prior `antigravity-cli` file loop above them.
|
||||
- No imports/variables orphaned by these changes.
|
||||
- No tests exist that exercise the agy arm of `provision_isolation` (the only `provision_isolation` test, `test_comp_create_isolation_folder_setup`, exercises the `claude` arm), 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]
|
||||
Reference in New Issue
Block a user