review: batch-end quality round#51
Merged
Merged
Conversation
Delete restated and section-label comments; tighten a WHY comment to its essential constraint without losing it.
Move Marks and guestPortConn.Write next to their type blocks so each type's method set stays contiguous; swap sort.Slice for slices.SortFunc+cmp.Compare, Printf with no verbs for Println, and add a return type hint to main().
Drop the epochMu/epochWritten dedup guard in persistEpoch: the only call sites (New, and UpdateSelf under updateMu held for the whole method) already serialize writes with a strictly increasing epoch, so the guard never rejects a write in practice. persistEpoch now just delegates to storeEpoch. Remove TestPersistEpochMonotonic, which existed only to exercise the removed guard by calling the unexported method out of the normal locking order.
handlePreview clamped the requested TTL against time.Until(deadline) unconditionally; for a claim with a zero deadline (e.g. an archived sandbox with ArchiveDeleteAfterSeconds=0) that clamp saturated to a deeply negative duration, so the minted token was already expired and every preview request 403'd on the first GET. Apply the lease ceiling only when the claim has one, and fall back to a default TTL otherwise.
The session exec path has no Started frame to answer a detached request: Session::run only ever emits stdout chunks and a terminal Exit/Internal frame, so detach=true with session set silently ran in the foreground instead of detaching, breaking any SDK caller (Go, Python) that sets both. Reject the combination up front with the same bad_request validation already used for an empty argv.
Guests running older silkd hang on detach+session instead of returning the BadRequest newer silkd responds with; failing fast in the SDK covers snapshot images that have not picked up the silkd fix. The Python spawn drops its never-usable session parameter for the same reason.
CMGS
added a commit
that referenced
this pull request
Jul 23, 2026
* review: comment budget pass Delete restated and section-label comments; tighten a WHY comment to its essential constraint without losing it. * review: declaration layout and modern idioms Move Marks and guestPortConn.Write next to their type blocks so each type's method set stays contiguous; swap sort.Slice for slices.SortFunc+cmp.Compare, Printf with no verbs for Println, and add a return type hint to main(). * review: loc-justify cuts Drop the epochMu/epochWritten dedup guard in persistEpoch: the only call sites (New, and UpdateSelf under updateMu held for the whole method) already serialize writes with a strictly increasing epoch, so the guard never rejects a write in practice. persistEpoch now just delegates to storeEpoch. Remove TestPersistEpochMonotonic, which existed only to exercise the removed guard by calling the unexported method out of the normal locking order. * fix(preview): mint a live token when a claim has no deadline handlePreview clamped the requested TTL against time.Until(deadline) unconditionally; for a claim with a zero deadline (e.g. an archived sandbox with ArchiveDeleteAfterSeconds=0) that clamp saturated to a deeply negative duration, so the minted token was already expired and every preview request 403'd on the first GET. Apply the lease ceiling only when the claim has one, and fall back to a default TTL otherwise. * fix(silkd): reject detach combined with session The session exec path has no Started frame to answer a detached request: Session::run only ever emits stdout chunks and a terminal Exit/Internal frame, so detach=true with session set silently ran in the foreground instead of detaching, breaking any SDK caller (Go, Python) that sets both. Reject the combination up front with the same bad_request validation already used for an empty argv. * fix(sdk): reject session in Spawn client-side Guests running older silkd hang on detach+session instead of returning the BadRequest newer silkd responds with; failing fast in the SDK covers snapshot images that have not picked up the silkd fix. The Python spawn drops its never-usable session parameter for the same reason.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Batch-end quality round across the Go, Rust and Python trees: style walkthrough, comment budget, simplify lenses, and a loc-justify pass with adversarial verification of every candidate.
Removed
sandboxd/mesh:Mesh.epochMuandMesh.epochWritten(8 lines, plus the test that existed only to drive them).persistEpochhas exactly two non-test call sites:New(), which is single-threaded and has not yet published the*Mesh, andUpdateSelf(), whose whole body runs underupdateMu. Both ofUpdateSelf's production callers funnel through that lock, sopersistEpochcan never run concurrently with itself. Theepoch <= epochWrittenbranch is therefore not merely race-free but never taken:New()writes withepochWritten == 0and a seeded epoch above the persisted floor, andUpdateSelfalways passes a strictly increasing value under the lock. Write order, durability-before-publish ordering and error propagation are unchanged; on-disk monotonicity is still pinned byTestUpdateSelfConcurrentDropsNothing. The droppedTestPersistEpochMonotoniccalled the unexported method concurrently, bypassing the public API — a trigger no caller can produce.Kept, after verification
closeWriteduplication betweensandboxd/egress/proxy.goandsandboxd/server/relay.go. Net gain is 3 lines while adding a cross-package import edge for a 4-line stdlib idiom with nothing to drift; a third identical copy insdk/gostays by the module boundary, so extraction would not produce a single source of truth; and the relay copy carries a tail-drain rationale the merged version would lose.sdk/goPtyOpts. Exported API of a separately published module (mcp/go.modande2e/go.modboth require it, documented in docs/sdk.md). Collapsing it intowire.PtyOpenis a breaking change that would push a protocol frame into the caller-facing input signature and freeze the SDK to the wire package's evolution.Gates
make go-lint(golangci-lint run +fmt --diff, GOOS=linux and darwin, all five Go modules): 0 issuesmake go-test(go test -race, all Go modules): passcargo fmt --check+cargo clippy --all-targets -- -D warnings(boot/init, silkd): passcargo test(boot/init 16 passed; silkd unit + every e2e suite — exec/find/forward/fs/git/lsp/pty/session/tree/watch — pass)ruff check sdk/python sdk/openai sdk/langchain mcp: all checks passedpytestsdk/python + sdk/langchain: 132 passedsdk/openaipytest could not run — it declaresrequires-python >= 3.10and depends onopenai-agents, neither available in the local environment. Reproduced identically on the branch base, so it is pre-existing and unrelated to this diff; no workaround was applied. CI covers it.go.mod/go.sum/Cargo.lock/Cargo.tomlunmodifiedNet: -16 prod, -27 test (the test drop is the removed epoch-monotonicity test plus comment trimming in the Rust e2e suites).