|
| 1 | +# Gemini Code Review: Upstream PRs #334 and #336 |
| 2 | + |
| 3 | +Reviewed via `gh pr diff <N> --repo dlorenc/multiclaude | gemini` |
| 4 | + |
| 5 | +## PR #334 — Session ID Fix (`fix(cli): generate new session ID when no history`) |
| 6 | + |
| 7 | +### Summary |
| 8 | +When `multiclaude claude` is run and no session history exists, generates a fresh UUID instead of reusing the old session ID. Fixes "Session ID is already in use" error after abnormal exits. |
| 9 | + |
| 10 | +### Verdict: LGTM |
| 11 | + |
| 12 | +| Area | Assessment | |
| 13 | +|------|-----------| |
| 14 | +| Correctness | Sound — new UUID on `!hasHistory`, reuse on `hasHistory` | |
| 15 | +| Error handling | `UpdateAgent` failure is non-fatal (warning + continue) — appropriate | |
| 16 | +| State consistency | `agent.SessionID` updated before persist — safe | |
| 17 | +| New dependency | `github.com/google/uuid` — standard, no concern | |
| 18 | + |
| 19 | +### Notes |
| 20 | +- Local variable `sessionID` introduced for clarity — slightly redundant with `agent.SessionID` but functionally correct |
| 21 | +- Risk: **Low** — isolated change, no new failure modes |
| 22 | + |
| 23 | +--- |
| 24 | + |
| 25 | +## PR #336 — Error Messages + Process Detection |
| 26 | + |
| 27 | +### Summary |
| 28 | +1. Lowercases error strings to satisfy `staticcheck ST1005` |
| 29 | +2. Adds pre-restart process detection via `signal(0)` on agent PID and tmux pane PID |
| 30 | + |
| 31 | +### Verdict: LGTM with one suggestion |
| 32 | + |
| 33 | +| Area | Assessment | |
| 34 | +|------|-----------| |
| 35 | +| signal(0) usage | Correct on Unix | |
| 36 | +| Zombie handling | Covered — signal(0) returns error for zombies | |
| 37 | +| Race conditions | Minimal window — acceptable for CLI | |
| 38 | +| GetPanePID | Confirmed exists in `pkg/tmux/client.go` | |
| 39 | +| Error format | Multi-line with `\n` — unconventional Go but good CLI UX | |
| 40 | + |
| 41 | +### Actionable: EPERM handling (medium priority) |
| 42 | + |
| 43 | +If `signal(0)` returns `EPERM`, process is running but owned by different user. Current code treats this as "not running" and proceeds, potentially starting a duplicate. |
| 44 | + |
| 45 | +```go |
| 46 | +// Current |
| 47 | +if err == nil { return fmt.Errorf("already running...") } |
| 48 | + |
| 49 | +// Suggested |
| 50 | +if err == nil || errors.Is(err, syscall.EPERM) { return fmt.Errorf("already running...") } |
| 51 | +``` |
| 52 | + |
| 53 | +Unlikely in normal usage (same user/machine) but is a correctness gap. |
| 54 | + |
| 55 | +### Minor notes (non-blocking) |
| 56 | +- `GetPanePID` errors silently swallowed — logging would help debugging |
| 57 | +- `repo` variable only needed for tmux check — slight scope expansion |
| 58 | +- Risk: **Low-Medium** — new code paths but failures handled gracefully |
0 commit comments