fix(runner): bound the agent socket path under the AF_UNIX sun_path cap

[mattwilkinsonn]

Ported from sealed #975 + #974, which cannot merge — Compass now lives here.

The bug

The agent socket path is RuntimeDir + /containers/<container>/agent.sock (host.go:291), where <container> is NamePrefix + the server-minted 32-hex agent account id. That tail is 69 bytes, so the default /run/compass lands at 81 of the 107 usable bytes and the operator's --runtime-dir budget is 38 on Linux, 34 on darwin/BSD.

Nothing bounded it on any hop: --runtime-dir flows from main.go through run.go to filepath.Join to lc.Listen unchecked.

Over the cap, bind(2) returns a bare EINVAL naming neither the limit nor the length. Worse, by then MkdirAll has already run and the 0700 directory survives — nothing in the socket's lifecycle removes it, since Close removes only the socket file and never its parent. Checking the length before any directory is created makes that leak moot rather than adding a teardown.

Why the cap is derived, not written down

The sun_path array is not one size across the platforms //go:build unix admits: 108 bytes on linux/solaris/illumos, 104 on darwin and the BSDs, 1023 on aix. No pair of hand-written numbers is correct, and an over-sized constant would admit exactly the path the kernel rejects at bind — the failure this check exists to prevent.

const sunPathMax = len(syscall.RawSockaddrUnix{}.Path) - 1 is not an approximation but exactly the bound Go's own wrapper enforces: syscall_linux.go:559 rejects n == len(Path) for a non-abstract name, syscall_bsd.go:191 rejects n >= len(Path) — both landing on len-1.

The same cap was silently breaking the runnerhub integration test

TestIntegrationProvisionStartRelayToStoreAndBus passed t.TempDir() as the RuntimeDir. t.TempDir() derives its path from the test name, and the 38-byte budget implies a 19-character ceiling that every one of the package's 43 tests exceeds (shortest: 26). Only this test failed, because only this one opens an agent socket — so the latent trap was the next test to wire a SessionHost, not a future long name.

It now uses a fixed short root and asserts the resulting longest path against the cap.

Verification

• Real Postgres, genuinely executing (not the pgtest.RequireDSN silent-skip): red before the fix — hangs to the 600s deadline; green after — 11.5s PASS. Reclaimed 2031 leaked podman volumes to get past num_locks (2048), which was skipping the suite while printing ok.
• Two mutations verified red: dropping the pre-check lets the path reach bind, failing the diagnostic assertions and the never-created-directory assertion; tightening the bound to >= refuses an at-cap path the kernel accepts.
• Full gate green: moon run :ci, 35 tasks, zero failures.

Reach beyond the runtime path

compass-server hit this same limit blocking a runnerhub pgtest on their T3 write-through — the fix unblocks that too.

Refs SEA-1440, SEA-1443

---

Path traversal, found by review and fixed here

The length guard alone was half a validation. The agent account id is interpolated into the container name (spec.go) and that name becomes a path segment of the socket (host.go). filepath.Join cleans, so a ../ in the id escapes RuntimeDir — verified under Go:

../../../../tmp/pwned  ->  /run/tmp/pwned/agent.sock          (25 bytes)
../../etc              ->  /run/compass/containers/etc/agent.sock

listenAgentSocket would then MkdirAll a 0700 directory there and bind. The length guard cannot catch this, because traversal SHORTENS the path — that first example sails past a 107-byte cap.

The three arguments my own comment made for the id being safe do not hold at this hop, and stating them made the gap look considered-and-closed:

• minted 32-hex — but the width is a property of the minting site; the Runner never re-checks it.
• FK-constrained — but the FK is enforced by RecordAgentContainer, which runs after hub.Provision has already created the directory and bound the socket.
• admin-only — narrows who, not what.

Length and shape are the same validation of the same input at the same hop, so validAccountID now refuses anything that is not a single safe path element, at the entry point in BuildSpec. Red-first: every traversal row fails without the guard.

Other review fixes

• padTo fails instead of skipping. Under a 100-byte TMPDIR both subtests skipped and the package printed ok — the only coverage of the guard's boundary, silently disabled. The sibling helper shortRuntimeDir already failed closed on the identical condition; they now agree.
• t.Cleanup(cancel) restored adjacent to context.WithCancel in the integration test: cancel was registered only inside runSessionsLoop, leaving ~60 lines of fixture setup that could t.Fatalf with the context never cancelled. LIFO ordering and the documented drain are unchanged; CancelFunc is idempotent.
• pgtest budget tied to the real minted id — an assertion after the fixture reddens if store ids widen, rather than letting the modelled budget go stale while the real path overruns.
• Comments trimmed. Dropped two stdlib line citations that pin the comment to a toolchain version nothing asserts, and corrected the "exactly the bound Go enforces" claim: AIX permits len(Path) and a Linux abstract name carries no terminator, so the constant is deliberately conservative at both edges. Neither reaches the Runner, which only binds filesystem paths.

Review record — three rounds, terminated at the K=3 bound

Run per skill://review: one adversarial Opus reviewer per round, spawn verified against the harness-written transcript each time (holds read, no spawns, no worker-agent prompt).

round	high	medium	low	what it found
1	1	3	4	Path traversal via the unsanitized account id; a test that silently self-disabled; a lost t.Cleanup(cancel); stale/overstated citations
2	0	1	3	The round-1 fix commit left two comments asserting the opposite of the code beside them
3	0	1	2	The round-2 fix commit did it again — the new guard's stated rationale was measurably false

Every finding is dispositioned: all fixed, none deferred, no follow-up issue owed.

Worth stating plainly rather than burying: this did not terminate on a zero-finding round. Rounds 2 and 3 each found a defect introduced by the previous round's fix — both comment-truth, never logic. The shipped guards were verified independently correct each round (conjuncts individually load-bearing, no bypass into the socket-path construction, every numeric and platform claim checked against the Go source), so the logic converged after round 1 and the prose took two more rounds to catch up. That pattern is the honest summary of this PR: I was repeatedly more confident in my explanations than the measurements justified.

The round-3 case is the clearest example. I wrote that a control character "fails much later, at bind, as a bare EINVAL." Measured against listenAgentSocket's own ordering:

"abc\x00def"    mkdir=EINVAL   bind=(never reached)
"abc\ndef"      mkdir=ok       bind=ok
"abc\x7fdef"    mkdir=ok       bind=ok
"abc\u202edef"  mkdir=ok       bind=ok
"abc\u0085def"  mkdir=ok       bind=ok

Wrong hop for the NUL, and simply false for the rest — they bind cleanly. The guard was right and its justification was not, which is the worse failure: an unsound reason invites a later reader to delete a guard that is load-bearing for a reason nobody wrote down. Stating the real reason (these characters reach the container name and the logs that quote it) also showed the predicate was wrong — r < 0x20 || r == 0x7f admits exactly the C1 controls and bidi overrides that spoof a rendered name. Widened to unicode.IsControl plus the format class, with rows for both.

Gate: local only — this repo has no CI

sealedsecurity/compass is not registered on Woodpecker, has no workflow config, and has no branch protection on main (gh api .../branches/main/protection → 404). The checks on this PR are Graphite mergeability and Greptile; neither builds anything. The evidence below is testimony, not a gate, and is labelled as such.

moon run :ci --force — forced, so nothing is a cached success string:

Tasks: 35 completed
 Time: 2m 1s 677ms

35 tasks, zero failed, zero task-level cache hits (the 2-minute wall time is the real build; devenv-fork:build alone is ~2m from cold).

Against a live Postgres (not the pgtest.RequireDSN silent-skip): TestIntegrationProvisionStartRelayToStoreAndBus PASS in 14.6s, having hung to the 600s deadline before the fix.

Both re-run after restacking onto 05da6b3 — the earlier green described a base that is no longer the merge tree. Main's four intervening commits are UI-only and touch none of this PR's five Go files, so the rebase was clean, but the evidence above is from the current head.

Every guard in this PR is mutation-verified red, not merely covered: dropping the length pre-check, tightening it to >=, dropping the -1, removing the traversal check, narrowing the control-character predicate, and weakening the NamePrefix check each redden a specific named subtest.

Filed, not fixed here

• SEA-1523 — pgtest silently skips when podman's lock table fills, reporting a green suite that ran nothing. Found running this PR's integration test: 2031 leaked dangling volumes hit num_locks (2048), and a fully-skipped pgtest still prints ok.

Round 4 — a discarded error, from a bot finding

Both receives on loopDone took the RunSessions error and dropped it. A stream
failure would have ended the loop while assertCleanShutdown still read as a clean
teardown — the error was the only thing distinguishing the ctx cancel ended it from
the stream died, and neither site looked at it.

Both now accept nil or context.Canceled and fail on anything else.

The predicate admits nil because I measured it, not because it is permissive.
Inverting the guard reddens the test and prints what the clean path actually
delivers:

--- FAIL: TestIntegrationProvisionStartRelayToStoreAndBus (11.84s)
    integration_pgtest_test.go:169: MUTATION probe: assertion reached, err=<nil>

That single run is doing two jobs: it proves the assertion is reachable (an
unreached assertion is the same green as a passing one), and it supplies the
value — a check for context.Canceled alone would have reddened every passing run.
Restored; pgtest green against live Postgres in 6.5s, gate 35/0.

[linear-code]

SEA-1440

SEA-1443

[mattwilkinsonn]

• fix(runner): bound the agent socket path under the AF_UNIX sun_path cap #12 : 2 dependent PRs (#24 , #25 ) 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Greptile Summary

The PR hardens and tests Runner agent-socket creation.

• Rejects AF_UNIX socket paths that exceed the platform-derived limit before creating directories.
• Validates account IDs and container-name prefixes as safe path elements.
• Uses a bounded runtime directory in the runnerhub integration test and strengthens its setup and teardown checks.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
go/internal/runner/gateway/socket.go	Adds a platform-derived AF_UNIX path-length guard before filesystem mutation.
go/internal/runner/gateway/socket_test.go	Covers accepted and rejected socket-path boundary conditions and verifies that rejected paths leave no directory.
go/internal/runner/spec.go	Validates both components of generated container names against traversal and misleading control or format characters.
go/internal/runner/spec_test.go	Adds coverage for unsafe account IDs and path-separator-bearing name prefixes.
go/internal/runnerhub/integration_pgtest_test.go	Uses a short runtime directory, verifies the modeled account-ID width, and now checks RunSessions errors during normal and cleanup paths.

_{Reviews (6): Last reviewed commit: "test(runner): assert the RunSessions loo..." | Re-trigger Greptile}