Repository navigation
fix(msb): verify post-hoc port forward listener - #530
Conversation
basiliskus
left a comment
basiliskus
left a comment
There was a problem hiding this comment.
Reviewed at 70f25bf875347f20b92f0c2c82f27d45214d6ad3.
Good fix, and the test is the right kind. The probe is bounded, the failure path cleans up after itself, and the new regression test genuinely fails without the change — I verified that against the merge-base rather than taking the green run on faith. One stale doc.
-
Minor, confirmed: ADR-0015 still describes the behaviour this PR replaces.
docs/adr/0015-msb-post-hoc-port-publish-via-ssh.md:132It still says the implementation only does
kill -0after the settle window, and that "a future enhancement could add a host-port probe" — which is exactly what_acq_msb_forward_listener_readynow is. Concrete consequence: a maintainer reading the ADR while debuggingacq portsconcludes the false-success symptom is an accepted limitation with an open follow-up, when it is fixed. Worth folding the probe and its fail-closed behaviour into the decision body, and the settle-window-only approach into the superseded/history part.
What I verified, beyond reading it.
The regression test is not vacuous. New tests copied onto the merge-base 33c83c5:
not ok 1 msb ports: publish generates+authorizes the acq key once, serves, and opens the -L tunnel
-> assert_regex 'BatchMode=yes' failed; base ssh line has no BatchMode
not ok 5 msb ports(S2): a forward with no host listener fails the publish, no state recorded
-> expected RC=1, base gives:
acq(msb): published host 127.0.0.1:54925 -> sandbox pbox 127.0.0.1:3000
RC=0
That second failure is the bug, reproduced: at base a publish with no host listener reports success and records state. At head both pass. This matters because the test asserts an absence of false success, which is the shape that passes vacuously — 71a19cd had to repair exactly that in 116-clone-option.bats.
Ran the file twice consecutively, both clean, which is cheap insurance for a port-probing test.
The probe itself: bash /dev/tcp/127.0.0.1/$port, up to ACQ_MSB_FORWARD_PROBE_ATTEMPTS:-20 attempts at 0.1s after the existing settle window. Bounded, so it cannot hang acq ports; no nc/ss dependency; the adapter is bash so /dev/tcp is fine.
I went looking for the false-success the probe could still admit — something else already listening on the requested host port — and it is covered, though indirectly: ssh runs with ExitOnForwardFailure=yes, so a bind collision makes ssh exit before the listener probe is consulted, and _acq_msb_forward_start checks kill -0 first and returns early. Worth knowing the guarantee comes from ssh's behaviour rather than from the probe distinguishing whose listener it found.
Failure path is clean: probe failure kills and waits the ssh pid, returns before _acq_msb_ports_record, and the caller then kills the msb ssh serve pid. No orphaned process, no state written.
The test-stub change also answers the flake question I had about port selection. The ssh stub binds a real listener on the requested -L host port with SO_REUSEADDR and tears it down, with STUB_SSH_NO_LISTENER=1 to model the failure — so there is no pick-a-free-port-then-release race between the harness and the code under test.
BatchMode=yes / NumberOfPasswordPrompts=0 / -n are consistent with this path: it already used -F none and IdentitiesOnly=yes, so user ssh config was never consulted and an interactive prompt could only ever hang a backgrounded tunnel.
Validation. Ran: full suite ./scripts/test-acq-bats → 545 ok / 0 not ok / 5 skips (2 pwsh not available, 3 sbx not on PATH — grammar acceptance UNVERIFIED); test/bats/112-msb-ports.bats twice at head, both clean; the same file plus scripts/test-acq-lib.sh against merge-base 33c83c5 for the base comparison above; bash -n; the repo ShellCheck batch per AGENTS.md (clean — shellcheck is present in my environment, so that is a real pass); gh pr checks 530 all passing.
Not run: ./scripts/verify-ports-live and ./scripts/verify-backends need a sandbox-capable host, so real tunnel behaviour against a live msb sandbox is unverified by me — everything above is stubbed. For a networking fix that is the gap worth naming: the probe's timing against a real ssh -L establishing through vsock is the one thing a stub cannot tell you, and 20×0.1s after the settle window is the parameter I would want confirmed on a real host before trusting it under load.
Verdict: approve, with the ADR update as a follow-up rather than a gate. Worth one live acq ports publish on a KVM-capable host to confirm the probe window is generous enough in practice.
Assisted-by: OpenCode [gpt_5_5_default_v2] change-review
🤖 I have created a release *beep* *boop* --- ## [4.0.0](v3.1.0...v4.0.0) (2026-10-06) ### ⚠ BREAKING CHANGES * **kits:** acq no longer installs podman/OCI tooling by default. Users who need docker, docker compose, or rootless podman when the base image doesn't provide it must opt into the oci-engine kit with `acq configure`, `--kit`, or `ACQ_EXTRA_KITS`. * **kits:** acq no longer performs native OpenCode npm fallback installation. OpenCode is supplied by the pinned opencode agent kit from agentic-coding-patterns v1.12.0, currently delivering OpenCode v2.0.22. If the kit is unavailable or invalid, acq fails closed instead of installing opencode-ai from npm. * **kits:** existing acq sandboxes with adapter-installed podman are not migrated in place. Recreate affected sandboxes or add the oci-engine kit before relying on OCI behavior after stop/start. ### Features * **acq:** choose the host port per sandbox, and let --publish pin it ([#526](#526)) ([410f332](410f332)) * **acq:** export ACQ_WORKSPACE and ACQ_CLONE into the guest on both backends ([#457](#457)) ([e0b9a3b](e0b9a3b)) * **acq:** interactive `acq configure` UI for extra kits + token-scoping defaults ([#500](#500)) ([13d9d3a](13d9d3a)) * **kits:** adopt pinned agent and capability kits ([#474](#474)) ([6a67063](6a67063)) * **secrets:** add Windows-native DPAPI secret backend ([#464](#464)) ([907f404](907f404)) * **windows:** add acq preview install path ([#463](#463)) ([d6d76f4](d6d76f4)) ### Bug Fixes * **acq:** use sandbox workspace for github scoping ([#476](#476)) ([1610835](1610835)) * **ci:** bump js-yaml to 4.3.2 to clear a high-severity CPU-DoS advisory ([#454](#454)) ([3217b49](3217b49)) * **ci:** bump smol-toml to 1.7.1 to clear an infinite-loop DoS advisory ([#459](#459)) ([c2291bf](c2291bf)) * **ci:** centralize markdownlint coverage ([#548](#548)) ([a27c31e](a27c31e)) * **ci:** pin markdownlint-cli2 once, clear GHSA-253c, unify file selection ([#527](#527)) ([e712685](e712685)) * **ci:** unblock unrelated PRs from the bash32 required-check deadlock ([#513](#513)) ([187aa57](187aa57)) * **install:** keep curl | sh stdin intact and tap msb formula first ([#462](#462)) ([c808d74](c808d74)) * **install:** trust the microsandbox-acq dependency before brew install ([#534](#534)) ([ffac0fa](ffac0fa)) * **kits:** land collapsed kit stack implementation ([#549](#549)) ([bcd5b9c](bcd5b9c)) * **msb:** allow Playwright browser downloads ([#477](#477)) ([0c399c5](0c399c5)) * **msb:** block unsafe 0.7 migration versions ([#510](#510)) ([3634046](3634046)) * **msb:** carry every origin URL, keep the scratch private, fail closed on a copy miss ([#488](#488)) ([babcd49](babcd49)) * **msb:** carry the source checkout's git identity into the --clone scratch ([#443](#443)) ([320d784](320d784)) * **msb:** follow the host resolvers and disable DNS rebind protection in the guest ([#444](#444)) ([d2f4bdc](d2f4bdc)) * **msb:** point the --clone scratch's origin at the source checkout's remote ([#455](#455)) ([7f48b94](7f48b94)) * **msb:** refresh ssh authorization for port publish ([#446](#446)) ([72a6bd5](72a6bd5)) * **msb:** session parity for exec/attach/shell (cwd, terminal identity, login shell) ([#427](#427)) ([6261eda](6261eda)) * **msb:** skip the recursive home chown when the image baked the agent user ([#432](#432)) ([d8f66a4](d8f66a4)) * **msb:** thread the merged kit env onto kit lifecycle commands ([#519](#519)) ([2248a92](2248a92)) * **msb:** translate kit wildcard network rules ([#451](#451)) ([e22bbc3](e22bbc3)) * **msb:** verify post-hoc port forward listener ([#530](#530)) ([8ca50e1](8ca50e1)) * **msb:** warn on broadened kit wildcard rules ([#460](#460)) ([6ded3ce](6ded3ce)) * **msb:** warn when ssh-agent route is missing ([#448](#448)) ([3a767a4](3a767a4)) * **sbx:** emit valid setup.install shape for the git-identity mixin ([#492](#492)) ([8e6160c](8e6160c)) * **sbx:** raise the version floor to 0.39.0, where sbx create --env first exists ([#491](#491)) ([30f9ecf](30f9ecf)) * **sbx:** wait for startup barrier before attach ([#512](#512)) ([33c83c5](33c83c5)) * **secret:** reject multiline test secret values ([#447](#447)) ([f2da950](f2da950)) * **secrets:** propagate rotation across backends ([#509](#509)) ([9641958](9641958)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Bret Mogilefsky <[email protected]>
Pin PATTERNS_KIT_REF to v1.17.0 release SHA a1206c52ec82ae7ce080ad44a6e6ac6f951e6d20. Includes merged Patterns #530 (feat: render neutral provider config). Update all dependent documentation examples and test assertions to match the new pin. No logic changes; kit functionality remains unchanged. Closes #625
## Before - PATTERNS_KIT_REF pinned to v1.16.1 (d2674db8b424402e912e95254d6b194a062deb73) ## After - PATTERNS_KIT_REF pinned to v1.17.0 (a1206c52ec82ae7ce080ad44a6e6ac6f951e6d20) - Includes merged [Patterns #530](GSA-TTS/agentic-coding-patterns#530) (render neutral provider config) ## Verification - ✅ v1.17.0 tag resolves and contains Patterns #530 artifacts - ✅ All literal assertions updated (3 documentation examples + 2 test assertions) - ✅ Focused tests pass: kit-ref-persistence, configure, pin/provenance - ✅ Static checks pass: gitleaks, markdown lint, diff audit - ✅ Blocker-only code review: PASS Closes #625 --------- Co-authored-by: OpenCode Agent <[email protected]>
Context
MSB post-hoc port publishing could report success after starting the background
ssh -Lprocess even when the requested host listener never became reachable. In practice this produced misleadingacq portsstate likesandbox 6767 -> host 127.0.0.1:6768 (post-hoc ssh -L)while127.0.0.1:6768still refused connections.Plan
Changes
ssh -n,BatchMode=yes, andNumberOfPasswordPrompts=0to the MSB post-hocssh -Lcommand.127.0.0.1:<host-port>after tunnel startup.Adversarial Review
A sub-agent review found one actionable issue: the initial tests used fixed host ports such as
8080and9090, which could collide with unrelated local services and mask the no-listener regression. This PR now allocates free loopback ports per test and explicitly skips the Bats file whenpython3is unavailable for the listener stub.Verification
Rollback
Revert this PR. Existing create-time
-ppublished ports are unaffected; only MSB post-hocacq ports --publishtunnel validation changes.Security Impact
Low positive impact. This keeps port exposure loopback-only and fail-closed, avoids interactive SSH auth hangs, and prevents recording unavailable tunnels as active. No changes to secrets handling, authentication flows, or external network destinations beyond the existing local SSH tunnel mechanism.
AI-assisted: yes