Repository navigation
Conversation
|
Claude finished @cweill's task in 18s —— View job Review of PR #859
FindingsNo blocking issues.
Minor, non-blocking notes
LGTM |
|
| result := sess.enqueueAttachWindowClient(cc, tt.ref, 0, 0) | ||
| if tt.wantErr != "" { | ||
| if result.err == nil || !strings.Contains(result.err.Error(), tt.wantErr) { | ||
| t.Fatalf("attach error = %v, want %q", result.err, tt.wantErr) | ||
| } | ||
| return |
There was a problem hiding this comment.
Rejected attach effects go unchecked
TestAttachWindowUsesExactName passes zero dimensions and checks only the error for duplicate names. It cannot catch an attach that resizes either window or registers the client before returning the ambiguity error.
Pass nonzero dimensions in the duplicate-name case, then assert that both windows keep their sizes and the client remains unregistered. This would protect the no-change behavior required for rejected attaches.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Motivation
Attaching a remote window named
2initially selects the correct window, but the mirror subscription and later layout reconciliation interpret that name as window index 2. The mirror can resize and switch to an unrelated window.Summary
WindowNamefield as an exact name, rejecting missing and duplicate names before subscribing or resizing.Testing
Confirmed both new regression tests fail on the original implementation.
Pre-push merged diff coverage: 100% (20/20 executable changed lines). The subsequent test-fixture-only commit passed its targeted tests 100 times; its push reused this coverage result without rerunning the pre-push hook.
TMPDIR=/tmp go test ./internal/server ./test -run 'TestAttachWindowUsesExactName|TestRemoteCLIAttachNumericWindowName' -count=100 -timeout 300sTMPDIR=/tmp go test ./test -run '^TestAttachWindow(StreamsLayoutUpdates|InitialResizeBroadcasts)$' -count=100 -timeout 300sTMPDIR=/tmp go vet ./...TMPDIR=/tmp go test ./internal/server ./internal/server/mirror ./test -run 'TestAttachWindow|TestRemoteCLI.*Window|TestWindowMirrorCheckpointAndRestore' -count=1 -timeout 60sThe full
TMPDIR=/tmp go test ./... -timeout 120srun reported failures in untouched audit-log descriptor, pprof socket-path, wedged-socket, chooser, and key-pacing tests. The audit-log, pprof, and dial timeout failures reproduce independently in packages unchanged fromorigin/main.CI is blocked at lint by pre-existing
SA1006ininternal/server/commands/layout/args.go:161(fmt.Errorf(remoteAttachUsage)). Confirmed the same line onorigin/main, with no diff to that file. Failed CI job.Manual review and simplification passes completed; Claude review returned
lgtm.Review focus
The mirror protocol already identifies windows by name. Ordinary CLI window resolution retains its existing index and prefix semantics. Check that both initial subscription and ongoing reconciliation use the exact name, and that an ambiguous name cannot resize either window.