Repository navigation
Conversation
…t the relay On a bridge egress lane the NIC is nft-locked at claim, yet silkd decided whether an exec gets http_proxy by asking whether a device-backed NIC exists, and a locked NIC still answers yes. Programs on that lane had neither a route nor the proxy variables; only a client pointed at 127.0.0.1:3128 by hand had network. The guest cannot see the nft hook, so the host says so: sandboxd writes an empty marker /run/silkd-nic-locked right after the readiness probe, in the two places it already installs the intercept CA (golden build, pre-golden cold boot), and silkd keys the exec environment on routes_directly() = has_egress() && !nic_locked(). Lane detection for git is untouched, so git still runs on the egress lane and now actually reaches the relay. Zero cost on the claim path; one stat per exec on a lane with a NIC until the first hit.
egresssmoke fetches the allowed origin with no -x, which is what shows the variables arrived. Its direct-egress probe now bypasses the proxy with --noproxy: with the variables present, a plain curl on the egress lane rode the relay and came back with the proxy's 403 body instead of timing out on the locked NIC.
One predicate, locksNIC, now decides the nft lock, the guest marker and the reconcile re-lock instead of three copies of the same two-field test, and markLockedNIC lives beside lockEgressNIC. A golden records whether it marked its guest in a .nic sidecar, so a node that moves from CNI networks to bridges rebuilds its egress-lane goldens instead of adopting ones whose guests would lock up without proxy variables — the same rule the CA sidecar applies. silkd drops the OnceLock latch around a tmpfs stat that cost less than the fork it precedes, keeps the marker path private, and names the environment rule after what it tests (routes directly, not has a NIC). The two lane tests fold into one table in egress_test.go next to the lock tests, and the marker's contract moves from the none-lane paragraph to the egress-lane section.
Codex round 1: /run is a tmpfs, so a guest reboot or a hypervisor watchdog reset recreated it while the host tap stayed locked, and a restarted silkd would have dropped the proxy variables again. The marker now lives at /etc/silkd-nic-locked on the guest's root filesystem, which the snapshot and a reboot both keep; promoted templates never carry it because the egress lane cannot promote. deploy.md's warmup row now names both lanes whose execs receive the proxy variables and says the relay is not armed until the claim.
The accept loop returned on any error that was not net.ErrClosed, and the pool discarded the return, so an EMFILE on a busy node killed the SOCKS5 door for the rest of the claim's life while the HTTP door on the same sandbox kept serving; http.Server backs off and retries in that case. The loop now does the same and ServeSOCKS returns nothing, since only the listener closing ends it.
resyncEgress locks a tap it finds open — a node that moved from CNI networks to bridges across a restart — without telling the guest, so silkd kept routing directly into a lock. The marker now follows the late lock, gated by the same open-tap test so an ordinary restart pays no RPC. Inline comments in the file move to the STE 101 register and quarantineClaim's godoc leads with its name.
A key such as "A B" survived shell quoting but failed bash's export, and the init script's exit code was discarded, so the session came up with the variable silently missing. session_create now refuses a name that is not a shell identifier with bad_request before spawning the shell; the hostile-key test that pinned the silent behavior asserts the refusal instead.
The same defect egresssmoke fixed: the image's proxy variables sent the plain curl through the intercepting relay, which answered, so the blocked-egress check could not pass. sockssmoke's last leg now says what it proves — no tunnel opens from a pool without a policy — instead of naming the host-side reason it cannot observe.
Inline WHY comments in pool.go move to the STE 101 register, two godocs that restated their identifier go, three multi-line comments in the smokes become one line each and drop issue-tracker narration, the find budget comment states the invariant it protects, Tunnels' godoc keeps two facts, the proxy's audit callback becomes the named AuditFunc like its sibling DialFunc, and the refill-concurrency table runs under t.Run like every other table in its file.
desktop.md now says guest-proxy.service tests NIC presence only and runs before the host can mark a locked NIC, so desktop pools belong on the none lane.
A readonly name such as UID passes the identifier check, so bash refuses the export and the session used to come up without the value; each export now exits the shell on failure like the cwd step, so create answers with an error instead.
The claim-shape advice said net=egress for web tasks, which on a guarded bridge gives the session neither a route nor the relay; the unit's NIC-presence test is the reason, not its start order.
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
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.
Whole-repo review round run on the state "#164 and #165 both merged". The branch starts from
feat/egress-socks5; its first four commits are #165's commits cherry-picked (they drop out once #165 lands and this branch is rebased), the ten after them are the round, three of them from the Codex passes on it (two rounds, converged with "No blockers remain"). Merge order: #164, #165, then this.Scope
The hygiene ledger decided what was read: 24 Go files due for the style lens and 17 for the judge lens (every file the two PRs touched plus what had moved since the 2026-09-11 round), every Rust and Python file changed since that round (9 silkd/boot sources, 10 silkd test files, 5 SDK/mcp modules and their tests), and all 18 docs pages in full. Readers: 3 Go style (sonnet), 2 Go judge/LOC (opus), 1 Rust source (opus, ownership-weighted), 1 Rust tests (sonnet), 1 Python (sonnet), 4 docs-drift (sonnet); every finding was re-read at the cited line before it was applied or declined. The ledger closes with 0 files due on both lenses.
Fixed (three defects, each in its own commit with a test)
sandboxd/egress/socks.go— the SOCKS5 accept loop returned on any error other thannet.ErrClosedand the pool discarded the return, so an EMFILE on a busy node killed the SOCKS5 door for the claim's life while the HTTP door (http.Server, which backs off) kept serving. The loop now backs off 50 ms and continues; only the listener closing ends it.sandboxd/pool/reconcile.go— the restart sweep locks an adopted egress claim's tap when it finds it open (a node that moved from CNI networks to bridges) but never told the guest, so silkd kept routing into the lock; the marker now follows the late lock, gated by the same open-tap test so an ordinary restart pays no RPC.silkd/src/session.rs— an env key such asA Bsurvived shell quoting but failed bash'sexport, and the init script's status was discarded, so the session came up with the variable silently missing.session_createnow refuses a name that is not a shell identifier withbad_request, and eachexportin the init script exits the shell on failure like thecdstep, so a name bash itself refuses (UID,BASHOPTS) fails the create instead of vanishing. The test that pinned the silent drop asserts the refusal; a second test pins the readonly case. This is a deliberate contract change from partial application to loud rejection.Also:
interceptsmoke's direct-egress probe now bypasses the proxy (the same defect #165 fixed inegresssmoke; verified on hardware, see below).Applied from the lenses
pool.goandreconcile.goin the STE 101 register, two restating godocs deleted, three multi-line smoke comments condensed and stripped of issue-tracker narration,Tunnels' godoc cut to two facts, the find-budget comment states the invariant it protects. Comment lines in the round: +19 / −25.type AuditFunc func(Event)inproxy.go(the func type was spelled twice; siblingDialFuncalready existed); the refill-concurrency table test runs undert.Runlike every other table in its file.-> Noneon every constructor,_peers -> list[str],_request(body: dict | None),__exit__(*exc: object).sockssmokeandqaab;desktop.mdsaysguest-proxy.servicetests NIC presence only, so it does not follow the lock marker, and its claim-shape advice now sends web tasks through a none-lane egress policy instead ofnet=egress(a real gap the docs reader found; teaching the desktop unit to wait for a lane verdict is a separate decision).Declined, with reasons
assert!without messages, fixed sleeps, a 7× three-line write block infs_e2e.rs— test-only, previous rounds accepted them, no rule broken.net_egress::serveandboot::runwithout///— one-line docs on public items are an allowance, not a requirement, and a review commit never raises a file's comment count.jsonalias onencoding/json/v2, membership-list loops withoutt.Run,Countersplacement, per-connectionreadNallocations, smoke poll loops that ignorectx— kept and recorded in the ledger.deploy.mdmakes draining old claims part of upgrading, the in-contract flip (CNI→bridge) is marked in the same sweep that locks, and an unconditional mark would make startup depend on every adopted guest answering a silkd RPC.LOC (loc-justify, exact counts)
Comments are 7.6 % of Go prod: comments are not the story. Growth since the base is features (
#148+232,#159qaab +199,#152desktop flavor +180,#161+180,#127+159,#129+156) while every review commit is net-negative (#126−624,#130−377,#154−87,#155−58). Per package the month is pool −222, types −79, store/peer −69, config −50 against egress +106 (the SOCKS5 door) and e2e +466.This round's own commits (ten): prod +80 −55 (net +25), test +36 −10 (net +26). Itemized prod adds: session-name validation and the export guard +14 (bug fixes), the
is_env_nameunit test insidemod tests+11 (counted as prod by the file split; test in substance),AuditFunc+3 (signature rule), reconcile late-mark +3 (bug fix), accept backoff +2 (bug fix); comment deletions −7. The two feature PRs together: prod +224 / test +325 against main.Gates
make go-lint: 10 ×0 issues.(5 modules × linux/darwin);make go-testall modules ok with-race;aslgate clean on both GOOS.cargo fmt --check,clippy -D warnings,cargo test110 passed on macOS and in arust:1.98.0linux/arm64 container; boot/init the same (16 passed).ruff format --check(28 files),ruff checkclean; pytest 156 / 6 / 5 (sdk/python, langchain, openai) on Python 3.12.rt-socks2:24.04guest image:scripts/intercept-e2e.shPASS (direct egress blocked with the fixed probe),scripts/socks-e2e.shPASS,scripts/sandboxd-e2e.shPASS. The silkd session change is covered by the crate's Linux test run; no guest image was rebuilt for it.