trunk-merge/pr-1041/b980ea91-3058-46a8-8d22-0bbc09f22f86 - #1054
Closed
trunk-io[bot] wants to merge 10 commits into
Closed
trunk-merge/pr-1041/b980ea91-3058-46a8-8d22-0bbc09f22f86#1054trunk-io[bot] wants to merge 10 commits into
trunk-io[bot] wants to merge 10 commits into
Conversation
…ncy (RIG-3437) The secrets WRITE path spawns the `secretspec` CLI by name (`internal/secrets/resolver.go`'s `defaultCLI`), but nothing guaranteed it on any shipped surface, and the guard that would have caught this skipped in CI — so the CLI half of the seam was asserted on a developer's box and guaranteed nowhere. The write path stays on the CLI: the pinned `secretspec-go` SDK is read-only (its native surface is `secretspec_resolve`/`call`/`abi_version`/`free`, no set/delete), so routing writes through it is not available at this pin. Linking the write path through `libsecretspec` is the long-term fix, filed as RIG-3581. Instead the CLI becomes a declared install-time dependency, the pattern this repo already uses for cloud-hypervisor, virtiofsd, passt and podman: - `hostcheck.SecretSpecFloor` pins 0.20.0, tracking the `go.mod` SDK pin so the read (SDK) and write (CLI) halves cannot drift. Deliberately NOT added to `MicroVMFloors`: that slice also feeds `VerifyMicroVMSupport`, which refuses Runner STARTUP, and boot reads through the SDK and never needs the binary. A test pins the separation. - `compass-stack preflight` reports it after the microVM trio, turning "the first admin write fails" into an install-time `[FAIL]` line. - `TestSecretSpecCLIVersionFloor` now FAILS CLOSED instead of skipping. At runtime an absent CLI is indistinguishable from a broken write path — both surface only as the first admin write failing — so a skip let the seam pass silently on exactly the lanes that do not stage it. - `tools/toolchain/secretspec-env.nix` stages it on the CI runner, so that fail-closed assertion actually runs green rather than reddening every lane. It is realized out-of-band from the `secretspec-nixpkgs` lock node for the same dotted-input reason as `chromium-e2e-env.nix` and `skopeo-nix2container-env.nix`: the reference lives outside `devenv.nix`'s parsed `packages` literal, so `parity.ts` never emits it. Staged in BOTH the `moon` job and the `pgtest` job — the latter runs `go test -tags pgtest ./...`, which includes `internal/secrets`. - `docs/self-host.md` documents it as a prerequisite with `brew install secretspec` (homebrew/core carries 0.20.0, bottled for macOS arm64 and Linux; the Linux bottle pulls dbus for the keyring provider). `devenv.nix`/`devenv.yaml`/`devenv.lock` are untouched: the dotted-input placement was already correct, and the gap was only ever on CI's side. Review-loop fixes folded in before first push: - The `microvm` CI job runs an UNSCOPED `go test -tags microvm ./...`, so it walks `internal/secrets` too — a build tag selects which FILES compile, not which PACKAGES `./...` visits, and `resolver_test.go` carries no tag. It stages the binary in-process alongside the VMM trio; without this the job reds on every push to main. - Comments no longer claim `Delete` spawns the CLI: it is a validate-only no-op (`resolver.go`), so only `Set` spawns. - `docs/self-host.md` no longer claims the nix flake channel provides the CLI. The flake carries no secretspec and its nixpkgs pin resolves 0.14.0, below the floor stated in the same paragraph. - `compass-go:test` now lists its own inputs including the helper, so moving the pin reschedules the battery instead of hitting a stale cache. Refs RIG-3437 Co-authored-by: Matt Wilkinson <matt@rigel.build>
T5 shipped six of its seven moves; move 6, the cross-turn causal link, never landed, so a reply chain had no recorded branch point and nothing read CommsCallRequest.trigger_traceparent outside generated code. RelayCommsCall had no span to hang a link on: the RunnerService door mounted only the bearer pair. Mount otelconnect on it, outermost of that pair, so the span envelopes the security-critical interceptors — the ordering serve.go states for every other chain. It is inert without a provider, so it mounts unconditionally like the rest. The Post arm then adds a Link from a valid trigger_traceparent. A LINK, never a parent: the reply keeps its own trace and merely references the trigger, which is what stops a conversation growing one unbounded tree. Parsing goes through the shipped W3C helper, but against a context stripped of its span. Reusing it on the handler ctx looks equivalent and is not: that helper returns its input unchanged on malformed input, so the local span survives, is valid, and the reply links TO ITSELF while passing an IsValid check. The malformed, bad-hex and all-zero cases pin that. Un-skip the two E2E assertions this blocked: the origin span is a fresh root, and a triggered reply starts a new trace carrying exactly one link back. Co-authored-by: Matt Wilkinson <matt@rigel.build>
Review round 1 found the origin span carries a link nobody wrote: otelconnect's server branch mints one from the inbound transport context whenever the caller propagated a traceparent, and the production Runner's client interceptor always does (runner.go:107-115, otelconnect interceptor.go:110-117). Measured: 2 links with a trigger, 1 with an empty one. The E2E fixture could not see it. It mounted otelconnect on the server only, so no traceparent ever arrived — the span's links were exactly what linkTrigger put there, and its fresh-root assertion passed because nobody offered a parent rather than because the door refuses one. Mount the client interceptor too, mirroring the real ServerLink client. Then stop reading the trigger link by position. Stamp it compass.link.kind=cross_turn_trigger and select on that, so both the assertion and any trace consumer stay right whatever else links the span; the negative control counts cross-turn links, not all links. The fresh-root invariant itself is unaffected: trustRemote is false, so otelconnect uses WithNewRoot and the reply never nests. Whether the record's 'empty adds no link' should be read as the span's total link set is a question about the frozen record, filed rather than decided here. Co-authored-by: Matt Wilkinson <matt@rigel.build>
Review round 2: the client interceptor that makes (h) and (i) non-vacuous was itself undefended. Remove it and every assertion stayed green — the parent check passes either way, and the attribute selector matches with or without a transport link — so the fix silently reverted to the topology that never ships. Assert the transport link's presence in (h), where the empty trigger means linkTrigger adds nothing and the count isolates it. Verified by dropping the client interceptor: the suite now reddens. (h)'s comment still claimed the Runner propagates no traceparent. It does; the span is a root because the door refuses the offered parent, which is the invariant worth stating — the old wording would survive a trustRemote flip, the exact mutation that must redden. Cite linkTrigger and Hub.RelayCommsCall by symbol: this PR's own doc-comment insertions had already pushed the line numbers off, one of them into prose. Note that the discriminator lives in link attributes, so zeroing the link attribute limit exports the edge without its kind. Co-authored-by: Matt Wilkinson <matt@rigel.build>
This PR's insertions moved relay_comms.go by three lines, which silently broke three citations pointing into it from other files: the reverse-binding note in store, and the sweep-enqueue and promote-deletes-the-binding notes in the E2E test. All three now name symbols. Checking them turned up a fourth error in the prose itself: the sweep note credited OnSessionStarted to a Hub.Start that does not exist. The call is in promoteSession. The remaining line-number citations into this file live in merged design records and were already stale before this change; they are not this PR's to rewrite. Co-authored-by: Matt Wilkinson <matt@rigel.build>
…874) Design record for the UI half of the J1 correlation-key seam: a sessionIdInterceptor in @compass/client puts the PostHog session id on outbound Connect requests, so the backend spans that already read X-POSTHOG-SESSION-ID carry semconv session.id. Supersedes the 2026-09-05 hold (RIG-3233). Both of its reasons are discharged: the inbound consumer merged (#996), and the boot-ordering objection is deleted by constructing analytics before the live clients rather than worked around with a mutable slot. The sender-side guard is printable ASCII plus a 200-char cap, which is stricter than the two limits the server enforces. Headers.set takes a WebIDL ByteString, so a well-formed id above U+00FF throws inside the interceptor and would fail the whole RPC, and U+0080-U+00FF rides as invalid-UTF-8 Latin-1 that the server silently drops. X-POSTHOG-DISTINCT-ID stays excluded: it identifies a person, and backend spans land in the trace store J1 keeps identity out of. Ledger: DL-346, DL-347. Co-authored-by: Matt Wilkinson <matt@rigel.build>
Round 3 lows. The (h) comment said the Runner propagates a traceparent unconditionally; otelconnect injects only what the propagator put in ctx, and the propagator is installed on the tracing-enabled path, so an untraced deployment mounts the interceptor and propagates nothing. The link-attribute-limit note lived only on the production constant, but the test helpers are what break if the attribute is stripped: the link survives and attribute selection then finds nothing. Say so where the selection happens. Co-authored-by: Matt Wilkinson <matt@rigel.build>
|
Compass engineering docs preview: https://trunk-merge-pr-1041-b980ea91.compass-eng-docs.pages.dev Deployed from Changed pages: |
trunk-io
Bot
deleted the
trunk-merge/pr-1041/b980ea91-3058-46a8-8d22-0bbc09f22f86
branch
September 9, 2026 21:35
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.
This pull request was created and is being managed by Trunk Merge.
This pull request is based on the main branch at SHA 1661a072af226be1441eabc2a4e613064de845de.
See more details about each PR in the batch here:
When CI completes, this pull request will be closed automatically.
Pull Requests Being Tested
This pull request is testing a batch with the changes from pull requests 1041, 1040, and 1043 - batching documentation.