fix(validate): judge the socket bound against the runtime catalog - #509
Open
schickling-assistant wants to merge 1 commit into
Open
fix(validate): judge the socket bound against the runtime catalog#509schickling-assistant wants to merge 1 commit into
schickling-assistant wants to merge 1 commit into
Conversation
#442 added a socket-path guard that read its bound off whatever tree was being validated. Publication does not validate the tree that will run: `st2 agent publish` admits a candidate through a disposable projection nested in the catalog's control directory, and validation canonicalizes that root, so every identity was charged for the projection's own depth. Declarations whose real socket is bindable were refused, and CI showed it on an 11-byte task id: agents/host/worker/agent.kdl [socket-path-too-long]: task 'host.worker' would bind a 108-byte session socket at .../catalog/.st2/catalog-admission-okX01u/pty/host.worker.sock On a host whose catalog root is 49 bytes the projection leaves a 15-byte identity budget, so once authoring resumed this would have failed closed on essentially every managed declaration. A fail-closed guard on the wrong measurement is worse than the defect it replaced. The bound is a host-local runtime fact, so it now travels with the caller that knows where the supervisor will bind sockets. `RuntimeRoot` makes that explicit in the validator's contract: `Catalog(root)` names the catalog that will run, and `Unknown` is the deliberately context-free mode that omits the host-local guard. Publication supplies the real catalog while validating the projection; catalog transactions supply it while validating captures, stages and admission projections; direct validation of a live catalog supplies itself. Two publish-path regression tests cover the boundary the original tests missed. Both pin `PTY_ROOT` off, because an ambient pty root wins over the catalog-relative default and makes the guard unobservable — which is why this regression was visible in CI and invisible locally. Refs #432
schickling-assistant
marked this pull request as ready for review
September 7, 2026 11:32
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.
Problem
#442 added a socket-path guard that read its bound off whatever tree was being validated.
Publication does not validate the tree that will run:
st2 agent publishadmits a candidate througha disposable projection nested in the catalog's control directory, and validation canonicalizes that
root, so every identity was charged for the projection's own depth.
mainwas red on both platforms, on an 11-byte task id:The CI failure is the mild version. On a host whose catalog root is 49 bytes:
So once authoring resumed, publication would have failed closed on essentially every managed
declaration, including ones already published. A fail-closed guard on the wrong measurement is worse
than the defect it replaced: #432 was one seat that could never spawn, this is every publication on
the host.
Goal
The guard judges the socket path the supervisor will actually bind, and the validator's contract
says whose job it is to know that.
Decisions
RuntimeRoot:Catalog(root)names thecatalog that will run;
Unknownis a deliberately context-free mode that omits the host-localguard. Publication supplies the real catalog while validating the projection, catalog transactions
supply it while validating captures, stages and admission projections, and direct validation of a
live catalog supplies itself.
st2 validateis not weakened — it still runs the guard..st2/catalog-admission-*ancestor wouldcouple correctness to a directory name that is free to change.
the parameter only makes explicit what each caller already knew.
Verification
Fail-before, via the faithful wiring mutant. The mutant makes the validator ignore the caller's
runtime root and measure the canonicalized tree under inspection, which is exactly the pre-fix
behaviour:
publication_judges_the_socket_bound_against_the_runtime_catalog_not_the_projectionpublication_still_refuses_an_unbindable_canonical_socket_pathThat second row is the point, and it is why a clause mutant would not have been enough: the
canonical-rejection test cannot see a wrong-root defect, because an identity that is over the limit
is over it under either root. Only the wiring test discriminates.
What each test would show under the opposite implementation. Test 1 fails with
socket-path-too-longnaming acatalog-admission-*path if the bound is ever read off theinspected tree again. Test 2 fails, by publishing successfully, if the guard is disabled broadly
rather than re-rooted — so the repair cannot be satisfied by removing the check.
Sensitivity to staging depth, not to this staging layout. Test 1 places the canonical socket
path 0, 4 and 8 bytes under the limit and publishes at each. Any nesting deeper than that headroom
trips the bound if it is measured, whatever it is called and however deep it is, so the test does
not encode the current layout.
Two instrument failures found and fixed before trusting any of this, both of which would have
produced a green that could not have gone the other way:
shadow.path()and survived. The control directory isa
/proc/self/fd/Npath — short until canonicalization resolves it — so that mutant was inert,not evidence of a sensitive test. The faithful mutant measures the canonicalized root.
PTY_ROOT, an ambientpty root wins over the catalog-relative default, and no catalog depth can then reach the limit.
That is why the regression was visible in CI and invisible locally. Both tests now pin
PTY_ROOTandPTY_SESSION_DIRoff, and the helper says why.Suites, targeted, in a warm target dir: lib 744 passed / 0 failed;
--test agent_publish17passed / 0 failed / 9 ignored — the 4 tests CI failed on now pass, and the 9 ignored are the
sandbox-gated set;
--test validate64 / 0;--test run60 / 0;--test invariants2 / 0.Held, not skipped: the full local Nix gate. Release condition — sequencing, since a heavy local
leg runs one at a time across the graph and this host is under load. CI will run once against this
candidate, which is the hermetic version of that evidence.
Complexity
One two-variant enum and one extra argument threaded to the call sites that already held the value.
No new dependency, no behaviour change for
st2 validate.Concerns
an explicit type with a documented
Unknownarm rather than anOption, and why the wiring mutantis part of the evidence rather than clause mutants alone.
agent_publishtests fail locally withroot-count: host 'host' must declare exactly one root agent; found 0. Pre-existing and unrelated — they are the set CI marks ignored, and theyfailed identically before this change.
sensitivity to the clauses I wrote; none of them exercised a caller that supplied the wrong root.
A mutation matrix measures clause sensitivity, not path coverage.
Friction & bottlenecks
PTY_ROOTbeing exported in the dev environment silently makes any socket-bound test vacuous.The new helper pins it, but nothing warns a future test author.
Follow-ups
References
Refs #432. Repairs the regression #442 introduced.
Posted on behalf of @schickling
agent_identitysessionagent_personaagent_supervisoragent_toolagent_tool_versionagent_runtimetooling_profile