Repository navigation
feat: list the 52d82f0a features in server.capabilities - #779
Conversation
The features array gains process.kill.farewell, files.list.includeDotNames, process.spawn.failure_reason and server.line_end at the places that 52d82f0a sends them. A Windows daemon with jobs on adds process.spawn.job last. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@claude review Generated by Claude Code |
There was a problem hiding this comment.
1 important, 0 nits
| Severity | File | Line | Finding |
|---|---|---|---|
| 🔴 Important | process_stdin_killwait_test.go |
468 | The guard "featuresFor changed the shared list" cannot fail. Without slices.Clip, the append writes into the spare capacity of capabilityFeatures, which slices.Contains never reads. |
I read the code. I did not run the tests, because CI is the measurement.
The wire change matches the PR description. capabilityFeatures at methods_server.go#L52-L68 puts the four names at the places the PR names. The two goldens, wantCapabilityFeatures and TestCapabilitiesAdvertisesNewSurface agree with that order. The Windows golden needs no process.spawn.job, because the socket harness builds its procManager with newProcManager (harness_test.go#L303-L308) and never calls startChildJobs. So jobs stays jobsOff there. The missing process.spawn.pty is already documented at docs/protocol/server.md#L160. Each new name has code behind it: farewell.go, IncludeDotNames in methods_files.go, spawnFailureReason in spawnreason.go, and lineend_test.go for the line end.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@claude review Generated by Claude Code |
There was a problem hiding this comment.
0 important, 0 nits
No important findings.
Previous findings
| Status | File | Line | Finding |
|---|---|---|---|
FIXED in a477bac |
process_stdin_killwait_test.go |
470 | The guard of the shared list could not fail. It now reads capabilityFeatures[:cap(capabilityFeatures)]. |
The fix holds. The shared list has spare capacity on every system. On Linux and macOS, the append of externalRootCapabilityFeatures to the 15-name literal grows the array, and server.instance_id then fits in it. On Windows, that append adds nothing, and the append of server.instance_id grows the array. In both cases, an append without slices.Clip in featuresFor (methods_server.go#L76-L81) writes the job name below the capacity, and the new check at process_stdin_killwait_test.go#L468-L472 reads it.
I read the code again at a477bac. I did not run the tests, because CI is the measurement. The rest of the diff is the same as in 5618aa9. The Windows tests of the full frame (the Windows golden, wantCapabilityFeatures and TestCapabilitiesAdvertisesNewSurface) build their server with newTestServer or newRunningServerAt. Neither calls startChildJobs (server.go#L643), so jobs stays jobsOff, and those frames correctly have no process.spawn.job.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… on Windows Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
server.capabilitiesnow lists the four features of build52d82f0athat claustrum has built, at the places the build sends them. On Windows, a daemon with jobs on also listsprocess.spawn.job.process.kill.farewellfollowsprocess.stdin.offset.files.list.includeDotNamesfollowsfiles.extract_tar.execBit.process.spawn.failure_reasonandserver.line_endfollowprocess.spawn.shellAgentSocket, beforelauncher.managed.process.spawn.jobcomes afterserver.instance_id. WithCLAUDE_SSH_NO_JOBS=1the name is not in the list.These positions were measured on
52d82f0aon Linux, macOS and Windows. claustrum does not listprocess.spawn.pty, because the terminal is not built yet.Wire surface
server.capabilitiesgains four names on every system, and the job name on Windows with jobs on. The twosocket_responsesgoldens change for that reason: the Linux one was regenerated with-update, and the Windows one gained the same four lines by hand.Docs
docs/protocol/server.md: the example array and one sentence on the Windows job name. Its word limit rises from 1490 to 1496, because the docs test pins the example array word for word.Test plan
gofmt -l .prints nothing.go vet ./...is clean for Linux, Windows and macOS.golangci-lint run ./...is clean.go test -race -timeout 20m ./...passes.TestCapabilitiesListTheJobNameOnlyWithJobspins the job name with jobs on and off.🤖 Generated with Claude Code