review: ledger round — sdk/go, sandboxd, e2e, silkd, python (4 fixes, 9 reviews) - #154
Merged
Merged
Conversation
Checkpoint.New applied every option into a full claim request but only the TTL reached the wire, so WithClaimRef was accepted and silently lost while WithVolumes, WithNetwork and WithSize were rejected. The claim_ref is what the k8s-side reverse lookup keys on; a silent drop leaves the sandbox unfindable. Reject it locally like the other inapplicable options; the server-side checkpoint claim carries no claim_ref field.
… stdlib folds drainProc and Run carried the same recv/switch loop; pumpStdio holds the union of the arms and Run keeps rejecting a bare done frame. Template.New had a hand-rolled no-volumes fast path that duplicated claimFollow's first contact with no_redirect pinned; the pin now rides the encoder closure, so a non-compliant redirect is rejected instead of yielding an empty handle. SetPoolsCluster dedupes with slices.Compact over a sorted copy, StartLsp uses oneShotRPC like every other one-reply verb, the peers forwarder folds into its one caller, and dialAgent drops the socket deadline that duplicated the AfterFunc close on the same ctx.
…dead guard The idle/archive sweep switches were derived three times (NewManager from the config specs, adoptPersistedPools from pools.json, SetPools from the live pool set); recomputeSweepFlags now derives them once from m.pools, after adoption and after every SetPools. poolWarmup's locked read moves into its only caller, claimsSnapshot forwarded to store.mark at both call sites, retryRemovals' empty-batch return duplicated what runBounded(0) already does, Info iterates a sorted maps.Values instead of building a filtered slice first, and mcp's str dropped a description guard no call site can reach (all 16 pass a literal).
…bound upgrade, nil-safe branch check crossnode published a checkpoint to the shared store and deleted it only on the success path; every node-B failure left it behind. egresssmoke and interceptsmoke defaulted -secret to the empty string, and strings.Contains(seen, "") made the injection assertion vacuously true, so a hand run printed PASS without proving injection; both now require the flag (egresssmoke only in the guarded mode, the negative control never reaches the check). rpcbench's copy of the SDK's upgrade dial never wired ctx to the socket, so a node that accepts TCP and never answers the upgrade blocked ReadResponse past the 5-minute budget. smoke's post-checkout branch assertion read br.Current in the error branch where br is nil.
The LSP framing is MIME-shaped (Key: Value lines terminated by a blank line), which textproto.Reader.ReadMIMEHeader already parses; the hand loop that trimmed CRLF and cut the Content-Length prefix goes. The length <= 0 guard stays so make cannot see a negative size.
fsChunk, readChunk and silkdChunk were three Go copies of silkd's BULK_CHUNK, and portWriteChunk was declared twice; wire already owns MaxFrame and every module imports it, so BulkChunk and PortWriteChunk live there with one comment each. The next sdk/go release cuts a wire tag alongside as the go.mod note requires.
… lock session_create wrote the env value POSIX-quoted but the key raw into the init script, so a key with a quote or newline broke the script before the marker printf and converse parked forever on a read with no timeout; the half-created session was in neither the table nor the reaper, so its bash leaked until silkd restarted. Quoting the key makes bash report "not a valid identifier" and continue, so the marker still arrives. lsp_start_language_name_cannot_escape reaches manifest_dir(), which reads SILKD_LSP_DIR, without holding ENV_LOCK, racing the set_var the other tests perform under it.
…t dedup kill_group and signal_pid each carried the same guard and a byte-identical SAFETY block; send_signal holds the one unsafe call and is_valid_pid's doc carries the pid-0 fact once. converse scanned the accumulator twice for the exit-code newline with the naive contains and position; memchr covers both, as the marker search already did. FIND_MAX_FILE is public so the e2e tests pin the bound to the real constant, and its doc no longer claims a binary check it does not make. sysutil hoists its two function- local use statements and drops the NSS_LOCK comment the static's doc and the SAFETY lines already carry; boot/init's inline comments join the lowercase register and persist_network's doc stops repeating the NIC_TIMEOUT fact.
…ions, folded preambles
common gains send, next_frame and DEADLINE, which eight sites in lsp_e2e,
exec_e2e and pty_e2e re-implemented as write-then-newline and
read-then-parse pairs. forward_e2e asserted on raw JSON text with a
hand-rolled substring extractor; it now parses frames like every other
file and shares one forwarded() handshake. exec_e2e's four poll-until-
exit loops and two ps polls become wait_for_exit and wait_listed;
lsp_e2e's four identical manifest-and-start preambles become
started_server; session_e2e's create takes no empty json!({}) at 13 of
14 sites and the busy-command spawn is one helper. find_e2e pins its
oversize fixtures to silkd::find::FIND_MAX_FILE instead of a literal,
git_e2e drops a last() wrapper the other nine files never had,
pty_e2e loses an assertion its own read_until predicate made
tautological, function-local use statements move to the top, and
tree_e2e sorts in place instead of cloning.
… a wedged child, the exec description states the inactivity bound CocoonSandboxSession.running caught APIError, SilkdError and OSError, but the commonest not-running path is a failed dial, which the SDK raises as ProtocolError; it and StreamTimeout escaped a predicate whose job is to answer the question. SandboxError is the base of all four. mcp/e2e.py's finally-close could raise TimeoutExpired over the AssertionError naming the failed step and left the child running; McpClient is a context manager that kills on timeout. The langchain exec description promised a 5-minute cut-off, but CALL_TIMEOUT is a socket inactivity bound.
…ts, annotated helpers test_client wrote the same recording claim handler seven times; recording_claim returns the seen list. test_wire_binding's three one-line stub wrappers fold into the CASES they serve, and the fixture helpers in test_wire_binding, test_proc and test_toolkit move below the tests they support. test_proc built a FakeConn it immediately replaced. client's three hardest helpers (_try_each, _redirect_fallback, _scatter) gain full annotations, the default retry policy becomes a named function, and their docstrings keep the policy while dropping the sentence that restated the name; sandbox annotates the stdio callbacks the public methods already type.
CI ran ruff check only, with a floating ruff; the /code-py gate is format --check + check + pytest at pinned versions. A root ruff.toml gives mcp/e2.py the same 120-column, same-rule configuration the three sdk packages carry in their pyproject files, and the workflow now fires on mcp/**/*.py.
With the stdio pump returning through the function-scope err, the inner err in the StdinClose branch shadowed it (govet shadow).
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.
Why
The hygiene ledger listed 51 production files (judge lens) and 11 files (style lens) whose blobs changed since the lenses last cleared them, plus the Rust and Python deltas since v0.1.9 that the ledger does not track. Readers covered every file in full; this PR carries the adjudicated findings. Every bug is its own
fix:commit ahead of thereview:commits; cut-list items stay in the round report for a decision.Fixes
sdk/go:Checkpoint.NewappliedWithClaimRefinto the claim request but never sent it, so the k8s-side reverse lookup keyed on it found nothing; it is rejected like the other inapplicable options, with a test.e2e: crossnode leaked the shared-store checkpoint on every node-B failure; egresssmoke and interceptsmoke passed vacuously when-secretwas empty (strings.Contains(x, "")); rpcbench's copy of the SDK upgrade dial had no ctx on the socket, so a node that accepts TCP and never answers blocked past the 5-minute budget; smoke readbr.Currenton the error branch wherebris nil.silkd:session_createwrote the env value quoted but the key raw, so a key with a quote or newline broke the init script before the marker andconverseparked forever on a read with no timeout, leaking the bash (test added); the LSP escape test readSILKD_LSP_DIRwithoutENV_LOCK.CocoonSandboxSession.running()letProtocolError(a failed dial, the commonest not-running path) escape;mcp/e2e.py's finally-close could mask the failing step and left a wedged child; the langchain exec description promised a wall-clock cut-off that is an inactivity bound (doc synced).Review
sdk/go:drainProcandRunshared one recv/switch loop →pumpStdio;Template.New's hand-rolled no-volumes fast path ridesclaimFollowwith the pin in the encoder;slices.Compact(slices.Sorted(...))dedupe;StartLspviaoneShotRPC;peersforwarder folded;dialAgentdrops the socket deadline duplicating theAfterFuncclose.sandboxd/mcp: the idle/archive sweep switches were derived at three sites →recomputeSweepFlagsover the live pool set;poolWarmup,claimsSnapshot, the empty-batch early return inretryRemovalsand mcp's unreachablestrdescription guard go;Infoiterates a sortedmaps.Values.protocol/wire:BulkChunkandPortWriteChunkreplace five Go copies of silkd's framing constants (the next sdk/go release cuts a wire tag alongside, as go.mod already requires).e2e/smoke: LSP headers vianet/textproto.ReadMIMEHeader.silkd/boot: onesend_signalwith one SAFETY block behindkill_group/signal_pid;memchrfor the exit-line scan;FIND_MAX_FILEpublic so tests pin the real bound; comment dedup and register fixes.silkd/tests:commongainssend,next_frame,DEADLINE; forward_e2e asserts on parsed frames instead of raw JSON substrings; exec/lsp/session preambles fold intowait_for_exit/wait_listed/started_server/create; function-localusehoisted.recording_claimhelper for seven copy-paste recorders, stubs inlined, helpers below tests, the three hardest client helpers annotated; CI gainsruff format --check, pinned ruff/pytest, and a rootruff.tomlsomcp/is governed.Numbers
Go prod comment lines 1505 → 1499; no commit adds a comment.
Evidence
Go:
GOWORK=off make go-lint100 issues.lines (5 modules × linux+darwin, fmt --diff clean);asl -forwarder=false ./...0 findings in every module on both GOOS;go test -race -count=1 ./...per module allok, 0 FAIL lines. Rust (pinnedrust:1.98.0container, linux/arm64): silkdcargo fmt --checkclean,cargo clippy --all-targets -- -D warnings0,cargo testevery suite ok (session suite 16/16 incl. the new hostile-env test); boot/init fmt/clippy/test clean. Python (3.12 venv):ruff format --check .andruff check .clean; pytest 155/5/6 passed across the three packages.Held for the owner (report-only cut-list): the buffered exec endpoint (−177, no first-party consumer),
DecodeStrictJSON→encoding/json/v2(−57, behaviour change on the operator surface), rpcbench mode B (−33), the browsersmoke/desktopsmoke HTTP helper (−23, needs a Chromium testbed run).