From 04cd79c0e0cd9418746e4db2ae602b0ed4c61d2b Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:31:38 +0200 Subject: [PATCH] fix(validate): judge the socket bound against the runtime catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 --- INVARIANTS.md | 2 +- docs/vrs/spec.md | 20 +++-- src/agent_author/declared_field.rs | 9 +- src/agent_publish.rs | 25 ++++-- src/catalog_graph.rs | 8 +- src/catalog_transaction.rs | 27 ++++-- src/validate.rs | 57 ++++++++++--- tests/agent_publish.rs | 133 +++++++++++++++++++++++++++++ 8 files changed, 247 insertions(+), 34 deletions(-) diff --git a/INVARIANTS.md b/INVARIANTS.md index 3b2a6580..51e2dae1 100644 --- a/INVARIANTS.md +++ b/INVARIANTS.md @@ -42,6 +42,6 @@ materialization, messaging, DING, or presence must preserve them. | **Atomic resource proposal publication** | Every changed resource publication is one host-owned compare-and-swap fenced by binding generation, state revision, and prior carrier digest. A persistent cross-process lock admits at most one proposal from the same prior. The content-derived proposal ID binds the accepted carrier digest and semantic outbox envelope; the durable intent becomes eligible only with the exact canonical carrier, then folds into one authoritative catch-up state. A pre-carrier crash exposes old state, a post-carrier crash catches up on restart, and retry after a lost acknowledgement returns the durable receipt without another transition. Ordinary reconciliation fails closed on out-of-band divergence after a committed intent; only an explicit generation-advance recovery may re-adopt the canonical carrier or its absence while invalidating the old intent and fence. | `src/resource_profile.rs::tests::atomic_publication_fences_races_and_survives_crash_restarts`; `src/resource_profile.rs::tests::generation_advance_explicitly_recovers_diverged_or_missing_carrier` | | **Pass progress is independent of publication** | A reconcile pass completes whether or not resync publication is making progress. Publication runs on its own thread, so a pass's per-seat watch-set handshakes never wait for a catalog lock, a stream lock, or a refused publication. One publication is outstanding per subscription at a time, and a queued publication is dropped when its subscription is deactivated or removed by a refresh, so nothing is published to a seat the pass has already decided receives no events. | `src/run/tests.rs::reconcile_pass_completes_while_a_resync_publication_is_blocked`; `src/resync/tests.rs::a_flush_never_hands_off_a_subscription_whose_publication_is_outstanding`; `src/resync/tests.rs::deactivation_drops_only_that_recipients_queued_publication`; `src/resync/tests.rs::a_refresh_drops_a_queued_publication_for_a_subscription_it_removed` | | **Terminal refusals are classified, not retried** | A refused resync publication is classified by what could admit it later. A recipient that is declared but not running parks its reservation: it is attempted once, captures and schedules nothing while parked, and re-arms with its exact reserved bytes when a refresh carries that recipient again. An ambiguous recipient, a foreign owning host, and an undeclared stream are permanently refused: the reservation is dropped and the carrier baseline advances so the same transition is not re-captured. Everything else, including an absent declaration, stays retryable. | `src/resync/tests.rs::a_not_running_recipient_parks_its_reservation_and_is_attempted_once`; `src/resync/tests.rs::a_parked_reservation_re_arms_and_replays_when_its_recipient_runs_again`; `src/resync/tests.rs::a_permanently_refused_reservation_is_dropped_and_not_recaptured`; `src/resync/tests.rs::refusals_are_classified_by_what_could_admit_them_later` | -| **Unbindable session sockets fail at admission** | Host-scoped validation rejects a pty task whose session socket path would exceed the portable 104-byte `sun_path` bound, because `pty` refuses that bind and the task can then never spawn. The bound is derived from the pty root resolved for the selected host rather than a fixed identity length, only that host's pty tasks are judged against it, and the diagnostic names the resolved path and the byte overage. A park whose cause is that same unbindable path does not advise `st2 unpark`, which would relaunch into the identical failure. | `tests/validate.rs::an_unbindable_session_socket_path_is_rejected_at_admission`; `tests/validate.rs::another_hosts_long_identity_is_not_judged_against_this_hosts_pty_root`; `tests/validate.rs::a_long_exec_task_id_is_not_a_socket_path_issue`; `src/run/tests.rs::session_socket_overage_is_derived_from_the_resolved_root`; `tests/run.rs::a_structurally_unrecoverable_park_does_not_advise_unpark` | +| **Unbindable session sockets fail at admission** | Host-scoped validation rejects a pty task whose session socket path would exceed the portable 104-byte `sun_path` bound, because `pty` refuses that bind and the task can then never spawn. The bound comes from the pty root resolved for the RUNTIME catalog the caller names, never from the tree under inspection: publication admits a candidate through a disposable projection nested in the catalog, and measuring that tree charges every identity for the projection's own depth and refuses declarations whose real socket is bindable. Only a deliberately context-free validation omits the guard. It is not a fixed identity length either, only that host's pty tasks are judged, and the diagnostic names the resolved path and the byte overage. A park whose cause is that same unbindable path does not advise `st2 unpark`, which would relaunch into the identical failure. | `tests/agent_publish.rs::publication_judges_the_socket_bound_against_the_runtime_catalog_not_the_projection`; `tests/agent_publish.rs::publication_still_refuses_an_unbindable_canonical_socket_path`; `tests/validate.rs::an_unbindable_session_socket_path_is_rejected_at_admission`; `tests/validate.rs::another_hosts_long_identity_is_not_judged_against_this_hosts_pty_root`; `tests/validate.rs::a_long_exec_task_id_is_not_a_socket_path_issue`; `src/run/tests.rs::session_socket_overage_is_derived_from_the_resolved_root`; `tests/run.rs::a_structurally_unrecoverable_park_does_not_advise_unpark` | | **Marker-matched declaration authority** | Every st2 write path that rewrites a declaration carrying `meta { managed-by "nix" }` admits it only from a caller asserting exactly the marker that declaration carries: the lifecycle verb and the compare-and-swap `agent publish` both refuse a mismatched marker, an unmarked subject, an unresolvable multi-marker declaration, and an empty or padded assertion before any write, while presentation, address, stream, and Resource authoring refuse a Nix-owned declaration outright. Publication reads that authority from the incumbent, so creating a declaration and republishing byte-identical bytes stay open — neither replaces bytes another writer owns. A marker-matched edit rewrites nothing but the lifecycle line and is admitted through the same full-catalog gate as a compare-and-swap publication of the same bytes, so it cannot commit a declaration the catalog would reject. | `src/agent_author.rs::marker_matched_lifecycle_authority_is_exact_and_source_preserving`; `src/agent_author.rs::marker_matched_retirement_refuses_a_candidate_admission_would_reject`; `tests/agent_desired_state.rs::cli_managed_by_authority_retires_a_projected_seat_and_refuses_every_inexact_claim`; `tests/agent_publish.rs::publication_honours_the_incumbent_ownership_marker`; `tests/agent_publish.rs::publication_refuses_an_unresolvable_owner_and_repairs_unreadable_bytes` | | **Proof references resolve** | Every qualified test named in this table exists in its named source file, so stale invariant claims fail the suite instead of silently surviving a refactor. | `tests/invariants.rs::qualified_proof_references_resolve` | diff --git a/docs/vrs/spec.md b/docs/vrs/spec.md index 536d35eb..51f709b4 100644 --- a/docs/vrs/spec.md +++ b/docs/vrs/spec.md @@ -418,12 +418,20 @@ Host-scoped validation rejects a pty task whose session socket path would exceed portable `sun_path` bound. `pty` binds `/.sock` and refuses a bind over the limit, so such a task can never spawn and fails identically on every reconcile pass, which also makes the pass result useless as a health signal for that -host. The bound is derived from the pty root resolved for the selected host, never a -fixed maximum identity length: the usable identity length is what remains of the limit -after that root. The portable 104-byte bound applies so a declaration admitted on Linux -does not fail on Darwin, and the diagnostic states the resolved path and the byte -overage so the author can shorten the identity rather than discover the failure as a -spawn error later. +host. The bound is derived from the pty root resolved for the RUNTIME catalog — the +one the supervisor will bind sockets from — and never from the tree under inspection, +which is frequently a different tree: publication admits a candidate through a +disposable projection nested in the catalog, transactions validate captures and stages, +and a retained catalog is addressed through a file-descriptor path that canonicalizes +back to the real one. A caller that knows where sockets will be bound supplies that +root; only a deliberately context-free validation omits the guard. Measuring the +inspected tree instead charges every identity for the depth of whichever temporary tree +is being validated and refuses declarations whose real socket is bindable. The bound is +never a fixed maximum identity length either: the usable identity length is what +remains of the limit after the runtime root. The portable 104-byte bound applies so a +declaration admitted on Linux does not fail on Darwin, and the diagnostic states the +resolved path and the byte overage so the author can shorten the identity rather than +discover the failure as a spawn error later. A park notice whose cause is structurally unrecoverable says so instead of offering `st2 unpark`, which would relaunch into the identical failure. The test is the same diff --git a/src/agent_author/declared_field.rs b/src/agent_author/declared_field.rs index 58f7926a..68cda9a0 100644 --- a/src/agent_author/declared_field.rs +++ b/src/agent_author/declared_field.rs @@ -246,7 +246,14 @@ fn refuse_address_collision( spec.address = requested.map(str::to_owned); } } - let report = crate::validate::validate_discovered(catalog_root, Some(this_host), &prospective); + // The prospective catalog is an in-memory edit of the live one, which is where sockets + // will be bound, so the live root is the runtime root. + let report = crate::validate::validate_discovered( + catalog_root, + Some(this_host), + crate::validate::RuntimeRoot::Catalog(catalog_root), + &prospective, + ); if !report .issues .iter() diff --git a/src/agent_publish.rs b/src/agent_publish.rs index 15f73456..0e89f5a5 100644 --- a/src/agent_publish.rs +++ b/src/agent_publish.rs @@ -494,8 +494,12 @@ fn verify_published_spec( observed_sha256 == expected_sha256 && observed == expected_bytes, "published Agent Spec readback mismatch: expected sha256 {expected_sha256}, found {observed_sha256}" ); - crate::catalog_transaction::validate_full_catalog(catalog) - .context("published catalog fails locked core/catalog re-admission")?; + // The live catalog is validated in place, so it is its own runtime root. + crate::catalog_transaction::validate_full_catalog( + catalog, + crate::validate::RuntimeRoot::Catalog(catalog), + ) + .context("published catalog fails locked core/catalog re-admission")?; Ok(observed_sha256) } @@ -576,8 +580,13 @@ fn read_regular_optional(path: &Path) -> Result>> { fn validate_overlay(catalog: &Path, control: &Path, candidate: &Candidate) -> Result<()> { let shadow = build_overlay(catalog, control, candidate)?; - crate::catalog_transaction::validate_full_catalog(shadow.path()) - .context("candidate fails full-catalog validation") + // The shadow is a disposable projection nested under the catalog; sockets will be bound from + // the real catalog, so that is the runtime root the candidate must satisfy. + crate::catalog_transaction::validate_full_catalog( + shadow.path(), + crate::validate::RuntimeRoot::Catalog(catalog), + ) + .context("candidate fails full-catalog validation") } fn build_overlay( @@ -653,8 +662,12 @@ pub(crate) fn admit_declaration_rewrite( .with_context(|| format!("create validation overlay {}", parent.display()))?; } fs::write(&target, bytes).context("write candidate into validation shadow")?; - crate::catalog_transaction::validate_full_catalog(shadow.path()) - .context("candidate fails full-catalog validation") + // Another disposable projection: judge the candidate against the catalog that will run. + crate::catalog_transaction::validate_full_catalog( + shadow.path(), + crate::validate::RuntimeRoot::Catalog(&catalog), + ) + .context("candidate fails full-catalog validation") } fn copy_filtered_catalog( diff --git a/src/catalog_graph.rs b/src/catalog_graph.rs index 8a9109bd..cdf80ed5 100644 --- a/src/catalog_graph.rs +++ b/src/catalog_graph.rs @@ -149,7 +149,13 @@ pub fn snapshot(root: &Path, this_host: &str) -> Result { let _lock = crate::CatalogLock::shared(&root) .context("acquire shared catalog-authoring lock for catalog graph")?; let found = crate::discover_strict(&root); - let report = crate::validate::validate_discovered(&root, Some(this_host), &found); + // The graph reads the live catalog in place, so the tree under inspection is the runtime root. + let report = crate::validate::validate_discovered( + &root, + Some(this_host), + crate::validate::RuntimeRoot::Catalog(&root), + &found, + ); let mut runtime_by_path: BTreeMap> = BTreeMap::new(); for row in crate::agents::roster_from_discovered(&found, &root, this_host) { diff --git a/src/catalog_transaction.rs b/src/catalog_transaction.rs index 1f85d3b9..2cbf92d3 100644 --- a/src/catalog_transaction.rs +++ b/src/catalog_transaction.rs @@ -398,7 +398,8 @@ pub fn diff(request: DiffRequest) -> Result { let before = project(&retained_catalog, ProjectionSource::Current, &catalog)?; validate_projection_link_counts(&retained_catalog, &before, "live catalog")?; validate_live_workspace_facts(&catalog, &before.workspace_dirs)?; - validate_full_catalog(&retained_catalog).context("validate live catalog for diff")?; + validate_full_catalog(&retained_catalog, crate::validate::RuntimeRoot::Catalog(&catalog)) + .context("validate live catalog for diff")?; anyhow::ensure!( before.root_sha256 == request.expect_sha256, "catalog diff precondition failed: expected root sha256 {}, found {}", @@ -409,7 +410,8 @@ pub fn diff(request: DiffRequest) -> Result { let captured = tempfile::tempdir().context("create prepared diff capture root")?; capture_prepared_catalog(&prepared, captured.path())?; let after = project(captured.path(), ProjectionSource::Prepared, &catalog)?; - validate_full_catalog(captured.path()).context("validate prepared catalog for diff")?; + validate_full_catalog(captured.path(), crate::validate::RuntimeRoot::Catalog(&catalog)) + .context("validate prepared catalog for diff")?; let before_specs = canonical_semantic_specs(&retained_catalog)?; let after_specs = canonical_semantic_specs(captured.path())?; @@ -1297,7 +1299,7 @@ pub fn bootstrap(request: BootstrapRequest) -> Result { let admission = tempfile::tempdir().context("create prepared-catalog admission root")?; materialize_projection(&desired, admission.path())?; - validate_full_catalog(admission.path())?; + validate_full_catalog(admission.path(), crate::validate::RuntimeRoot::Catalog(&catalog))?; let desired_config = crate::catalog::load(admission.path())?; validate_external_pty_root( &catalog, @@ -1337,7 +1339,7 @@ pub fn bootstrap(request: BootstrapRequest) -> Result { desired.root_sha256, staged.root_sha256 ); - validate_full_catalog(&stage)?; + validate_full_catalog(&stage, crate::validate::RuntimeRoot::Catalog(&catalog))?; let lock = initialize_bootstrap_control(&stage)?; sync_tree_dirs(&stage)?; Ok(lock) @@ -1405,7 +1407,7 @@ fn inspect_existing_bootstrap( &catalog, &desired.workspace_dirs, )?; - validate_full_catalog(&retained_catalog)?; + validate_full_catalog(&retained_catalog, crate::validate::RuntimeRoot::Catalog(&catalog))?; anyhow::ensure!( current.root_sha256 == desired.root_sha256, "catalog bootstrap target already exists with root sha256 {}, expected {}", @@ -1616,7 +1618,7 @@ pub fn apply(request: ApplyRequest) -> Result { // are mirrored as empty directories; their live content is never copied or hashed. let admission = tempfile::tempdir().context("create prepared-catalog admission root")?; materialize_projection(&desired, admission.path())?; - validate_full_catalog(admission.path())?; + validate_full_catalog(admission.path(), crate::validate::RuntimeRoot::Catalog(&catalog))?; let desired_config = crate::catalog::load(admission.path())?; validate_external_pty_root( &catalog, @@ -1728,7 +1730,8 @@ pub fn apply(request: ApplyRequest) -> Result { staged.root_sha256, verified.root_sha256 ); - validate_full_catalog(&catalog).context("validate applied live catalog")?; + validate_full_catalog(&catalog, crate::validate::RuntimeRoot::Catalog(&catalog)) + .context("validate applied live catalog")?; sync_dir(&catalog)?; generation.commit()?; test_checkpoint("before-clear"); @@ -1755,7 +1758,13 @@ pub fn apply(request: ApplyRequest) -> Result { } /// Full structural and host-scoped validation for a complete prospective catalog. -pub(crate) fn validate_full_catalog(root: &Path) -> Result<()> { +/// +/// `runtime` names the catalog whose resolved pty root bounds session sockets. It is a separate +/// argument because `root` is frequently NOT that catalog: admission validates a projection, diff +/// validates a capture, bootstrap validates a stage, and a retained live catalog is addressed +/// through a file-descriptor path. Reading the bound off `root` charged declarations for the depth +/// of whichever temporary tree happened to be under inspection. +pub(crate) fn validate_full_catalog(root: &Path, runtime: crate::validate::RuntimeRoot<'_>) -> Result<()> { let found = crate::discover(root); let mut hosts = BTreeSet::new(); for spec in &found.specs { @@ -1775,7 +1784,7 @@ pub(crate) fn validate_full_catalog(root: &Path) -> Result<()> { .map(format_issue), ); for host in hosts { - let report = crate::validate::validate_for_host(root, &host); + let report = crate::validate::validate_for_host_at(root, &host, runtime); errors.extend( report .issues diff --git a/src/validate.rs b/src/validate.rs index 33c5bd4c..18f99bea 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -110,26 +110,56 @@ impl Report { } } +/// Where the supervisor will bind session sockets for the catalog being validated. +/// +/// The validated tree is not always the tree that will run. Publication admits a candidate through +/// a disposable projection, and catalog transactions validate captures and stages; none of those +/// trees has a `pty` directory any session is ever bound at, and a retained live catalog is named +/// through a file-descriptor path whose length is unrelated to the real one. A socket-path bound is +/// therefore a host-local runtime fact that travels with the caller who knows it, never something +/// read off the tree under inspection. +#[derive(Debug, Clone, Copy)] +pub enum RuntimeRoot<'a> { + /// The catalog that will actually run. Its resolved pty root bounds every session socket. + Catalog(&'a Path), + /// No runtime context is available, so host-local socket-path checking is deliberately + /// omitted. Structural checks are unaffected. + Unknown, +} + /// Validate a catalog. Returns every issue found, in a stable order (files sorted by discovery). pub fn validate(root: &Path) -> Report { - validate_scoped(root, None, false) + validate_scoped(root, None, false, RuntimeRoot::Unknown) } /// Validate a whole catalog while checking host-local filesystem facts only for `this_host`. /// /// Structural checks remain fleet-wide. This scope only prevents a synced multi-host catalog from -/// warning that another machine's external workspace or task cwd is absent locally. +/// warning that another machine's external workspace or task cwd is absent locally. `root` is both +/// the tree under inspection and the runtime root, which is the ordinary case: a real catalog in +/// the place it will run from. pub fn validate_for_host(root: &Path, this_host: &str) -> Report { - validate_scoped(root, Some(this_host), false) + validate_scoped(root, Some(this_host), false, RuntimeRoot::Catalog(root)) +} + +/// [`validate_for_host`] for a tree that is not the one that will run: a publication projection, a +/// capture, or a stage. The caller supplies the catalog whose resolved pty root bounds sockets. +pub fn validate_for_host_at(root: &Path, this_host: &str, runtime: RuntimeRoot<'_>) -> Report { + validate_scoped(root, Some(this_host), false, runtime) } /// Validate a catalog from fail-closed discovery. Unreadable entries and directory traversal /// failures become attributed issues instead of silently narrowing the validation universe. pub fn validate_strict_for_host(root: &Path, this_host: &str) -> Report { - validate_scoped(root, Some(this_host), true) + validate_scoped(root, Some(this_host), true, RuntimeRoot::Catalog(root)) } -fn validate_scoped(root: &Path, this_host: Option<&str>, strict_discovery: bool) -> Report { +fn validate_scoped( + root: &Path, + this_host: Option<&str>, + strict_discovery: bool, + runtime: RuntimeRoot<'_>, +) -> Report { // Canonicalize so `$CATALOG`-rooted paths expand to absolute paths (a relative root would make // every `$CATALOG/...` look relative). Falls back to the given root if it does not exist yet. let root = &root.canonicalize().unwrap_or_else(|_| root.to_path_buf()); @@ -138,7 +168,7 @@ fn validate_scoped(root: &Path, this_host: Option<&str>, strict_discovery: bool) } else { discover(root) }; - validate_discovered(root, this_host, &discovered) + validate_discovered(root, this_host, runtime, &discovered) } /// Validate one caller-held immutable discovery result. Catalog graph readers use this to keep @@ -146,6 +176,7 @@ fn validate_scoped(root: &Path, this_host: Option<&str>, strict_discovery: bool) pub(crate) fn validate_discovered( root: &Path, this_host: Option<&str>, + runtime: RuntimeRoot<'_>, d: &Discovered, ) -> Report { let root = &root.canonicalize().unwrap_or_else(|_| root.to_path_buf()); @@ -444,12 +475,18 @@ pub(crate) fn validate_discovered( // A pty task whose session socket path exceeds the portable `sun_path` bound can never // spawn: `pty` refuses the bind, so the failure repeats on every reconcile pass forever // and makes the pass result useless as a health signal. Admission is the only place where - // it is cheap, attributable, and fixable by the author. Host-scoped, because the bound - // comes from the pty root resolved on the host that would run the task. - if let (Some(host), Some(Ok(compiled))) = (this_host, &compiled) + // it is cheap, attributable, and fixable by the author. + // + // The bound comes from the RUNTIME root, never from the tree under inspection. Publication + // validates a disposable projection nested inside the catalog, so measuring the inspected + // tree charged every identity for the projection's own depth and rejected declarations + // whose real socket is bindable. Host-scoped for the same reason it is runtime-scoped: the + // bound belongs to the host that would run the task. + if let (Some(host), Some(Ok(compiled)), RuntimeRoot::Catalog(runtime_catalog)) = + (this_host, &compiled, runtime) && runs_on_selected_host { - let pty_root = crate::run::effective_pty_root(root); + let pty_root = crate::run::effective_pty_root(runtime_catalog); let bus_id = compiled.bus_id(host); for task in &compiled.tasks { if task.kind != agent_spec::spec::TaskKind::Pty { diff --git a/tests/agent_publish.rs b/tests/agent_publish.rs index cf3e2354..bb29dd80 100644 --- a/tests/agent_publish.rs +++ b/tests/agent_publish.rs @@ -1571,3 +1571,136 @@ fn wait_for_path(path: &Path) { thread::sleep(Duration::from_millis(10)); } } + +/// `publish` with the pty-root environment pinned OFF, so the guard resolves the catalog-relative +/// default rather than whatever `PTY_ROOT` the invoking shell exports. +/// +/// Without this the socket-bound tests are vacuous wherever `PTY_ROOT` is set — an ambient root +/// wins over the catalog-relative one, so no catalog depth can reach the limit and the wiring under +/// test is unobservable. The Nix builder exports neither variable, which is why the regression this +/// repairs was visible in CI and invisible locally. +fn publish_with_catalog_relative_pty_root( + catalog: &Path, + spec: &Path, + expectation: &[&str], +) -> Output { + let input_sha256 = sha256(&fs::read(spec).unwrap()); + st2() + .env_remove("PTY_ROOT") + .env_remove("PTY_SESSION_DIR") + .args([ + "agent", + "publish", + "--catalog", + catalog.to_str().unwrap(), + "--spec", + spec.to_str().unwrap(), + ]) + .args(["--input-sha256", &input_sha256]) + .args(expectation) + .arg("--json") + .output() + .unwrap() +} + +/// A catalog root padded to exactly `target_len` bytes, so a test can place the canonical session +/// socket path at a chosen distance from the platform limit. +fn catalog_root_of_length(temp: &Path, target_len: usize) -> PathBuf { + let mut root = temp.join("catalog"); + let mut guard = 0; + while root.as_os_str().len() < target_len { + let missing = target_len - root.as_os_str().len(); + // One path component per pass; `/x` is the smallest step, so any remainder is reachable. + let name = "p".repeat(missing.saturating_sub(1).max(1)); + root = root.join(&name[..name.len().min(missing.saturating_sub(1)).max(1)]); + guard += 1; + assert!(guard < 64, "could not pad a catalog root to {target_len} bytes"); + } + assert_eq!( + root.as_os_str().len(), + target_len, + "padding overshot: {}", + root.display() + ); + fs::create_dir_all(&root).unwrap(); + root +} + +/// Publication must judge the socket-path bound against the catalog that will RUN, not against the +/// disposable admission projection it validates. +/// +/// `st2 agent publish` validates a shadow catalog nested inside the live one, so measuring the tree +/// under inspection charged every identity for the projection's own depth and rejected +/// declarations whose real socket is bindable. On the pre-fix implementation this fails with +/// `socket-path-too-long` naming a `catalog-admission-*` path. +/// +/// The fixture is deliberately sensitive to ANY staging depth rather than to the current layout: +/// the canonical socket path is placed within a few bytes of the limit, so any nesting deeper than +/// that headroom — whatever it is called and however deep it happens to be — would trip the bound +/// if the implementation measured it again. +#[test] +fn publication_judges_the_socket_bound_against_the_runtime_catalog_not_the_projection() { + for headroom in [0_usize, 4, 8] { + let temp = tempfile::tempdir().unwrap(); + let socket_len = st2::run::PORTABLE_SOCKET_PATH_LIMIT - headroom; + // Everything the canonical socket path adds after the catalog root: the pty directory, the + // separators and the `.sock` suffix. Probed through the shipped path construction rather + // than spelled out, so the fixture cannot drift from it. + let suffix = st2::run::session_socket_path(&Path::new("x").join("pty"), "host.worker") + .as_os_str() + .len() + - 1; + let catalog = catalog_root_of_length(temp.path(), socket_len - suffix); + + let canonical = st2::run::session_socket_path(&catalog.join("pty"), "host.worker"); + assert_eq!( + canonical.as_os_str().len(), + socket_len, + "fixture must place the canonical socket {headroom} bytes under the limit" + ); + assert!( + st2::run::session_socket_overage(&catalog.join("pty"), "host.worker").is_none(), + "fixture precondition: the canonical socket must be bindable" + ); + + let spec = temp.path().join("candidate.kdl"); + fs::write(&spec, "agent \"worker\" {\n host \"host\"\n argv \"true\"\n}\n").unwrap(); + let output = publish_with_catalog_relative_pty_root(&catalog, &spec, &["--expect-absent"]); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !stderr.contains("socket-path-too-long"), + "headroom {headroom}: a bindable canonical socket must not be refused: {stderr}" + ); + assert!(output.status.success(), "headroom {headroom}: {stderr}"); + } +} + +/// The guard still rejects a declaration whose CANONICAL socket path exceeds the limit, through the +/// same publish path. Without this, the repair above could be satisfied by disabling the check. +#[test] +fn publication_still_refuses_an_unbindable_canonical_socket_path() { + let temp = tempfile::tempdir().unwrap(); + let catalog = temp.path().join("catalog"); + fs::create_dir(&catalog).unwrap(); + let identity = "w".repeat(200); + let spec = temp.path().join("candidate.kdl"); + fs::write( + &spec, + format!("agent \"{identity}\" {{\n host \"host\"\n argv \"true\"\n}}\n"), + ) + .unwrap(); + + assert!( + st2::run::session_socket_overage(&catalog.join("pty"), &format!("host.{identity}")) + .is_some(), + "fixture precondition: this canonical socket must exceed the limit" + ); + + let output = publish_with_catalog_relative_pty_root(&catalog, &spec, &["--expect-absent"]); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(!output.status.success(), "stdout: {}", String::from_utf8_lossy(&output.stdout)); + assert!( + stderr.contains("socket-path-too-long"), + "an unbindable canonical socket must still be refused: {stderr}" + ); +}