From 8f7f4ed86873822ad3e986a209291d7d16b3f13a Mon Sep 17 00:00:00 2001 From: Godopu Date: Wed, 22 Jul 2026 08:54:23 +0900 Subject: [PATCH] docs(reports): add durable peer review reports from multi-agent loop --- .../report-e1c4d86b.md | 46 +++++++++ .../report-5157a4a7.md | 99 +++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 .agents/reports/canary-projects-multi-agent-mux-creator-claude/report-e1c4d86b.md create mode 100644 .agents/reports/canary-projects-multi-agent-mux-creator-cline/report-5157a4a7.md diff --git a/.agents/reports/canary-projects-multi-agent-mux-creator-claude/report-e1c4d86b.md b/.agents/reports/canary-projects-multi-agent-mux-creator-claude/report-e1c4d86b.md new file mode 100644 index 0000000..e8780ef --- /dev/null +++ b/.agents/reports/canary-projects-multi-agent-mux-creator-claude/report-e1c4d86b.md @@ -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._.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,"` 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] diff --git a/.agents/reports/canary-projects-multi-agent-mux-creator-cline/report-5157a4a7.md b/.agents/reports/canary-projects-multi-agent-mux-creator-cline/report-5157a4a7.md new file mode 100644 index 0000000..d092b8c --- /dev/null +++ b/.agents/reports/canary-projects-multi-agent-mux-creator-cline/report-5157a4a7.md @@ -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._.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=`), 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 ` 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 `` 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,"` (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] \ No newline at end of file