Repository navigation
Conversation
Ports forwarded by Docker Desktop, OrbStack or colima used to scatter across Apps and Dev Servers, named after the forwarder process. A forwarded port now lands in its own Containers section that names the container, locates its compose project or bind mounts, and closes it with docker stop instead of a signal. - Resolve host ports to containers through the Engine API over the daemon's unix socket (one ~10 ms call, no process spawn), falling back to docker ps and then to the runtime's name. The daemon chunks its responses even over HTTP/1.0, so the socket client decodes chunked framing; socket and CLI failures back off separately and a socket failure never disables the CLI fallback. - compose containers sit in one row named after their project with each container as its port row; a plain docker run without --name is listed by its image, since a generated name says nothing. - The port page reveals the compose directory or a mounted host directory in Finder and offers Stop Container, the one shell action: a review page (restart-policy note included) that runs docker stop through the CLI and verifies the port went free. Signalling the forwarder stays refused - it only breaks the mapping while the container keeps running.
|
SonghaiFan
left a comment
SonghaiFan
left a comment
There was a problem hiding this comment.
Changes requested — reviewed head 0ff5d5c against fetched main c85c28a.
This is not safe to merge yet. The blockers below can stop a different container from the one reviewed or report a port as free without verifying it.
Integration with current main:
- #7 and #8 are now closed, not recorded as merged PRs. Main nevertheless contains the favicon/close-impact safety implementation and Portless integration (#8 head feba7fe is an ancestor).
- A local trial merge has actual conflicts in MenuPanel.swift (listener grouping and fixed-address row fields) and Models.swift (mutable inference/editorPreview). I resolved these locally for inspection, preserving listenerGroupID preview isolation, container subgroups, editorPreview, and fixedName/fixed-address subtitles. This resolution is not pushed or Swift-validated.
- The existing CloseService identity/expanded-impact checks remain in the trial merge, but the new container-stop path bypasses equivalent checks.
Validation performed on that local trial integration:
- python3 -m unittest discover -s Tests/ReleaseTests: 4 passed.
- Pinned runtime preparation, including checksummed downloads and guarded Portless adaptations: passed.
- node --test Tests/PortlessTests/*.test.mjs: 11 passed, 0 skipped on Linux / Node 24.19.0. This is supplemental, not the CI Darwin Node 24.14.0 result.
- swift test: blocked (swift not installed).
- LEFTOPEN_OUTPUT_DIR=dist/ci zsh Scripts/build-app.sh: runtime preparation passed, then stopped at swift build because swift is unavailable. No app/CLI build, signing, macOS UI, or live Docker/OrbStack/colima validation is claimed.
- Trial integration diff against main passed whitespace checking.
- The GitHub connector returned no pull-request workflow runs for this PR head; this is not a passing CI result. Rebase/merge current main and require the actual macos-15 CI job to execute before merging.
Additional verification gaps:
- The always restart-policy warning is present, but policy inspection can use a different daemon/name from discovery. Fetch policy for the pinned identity, and show an explicit unknown-policy warning on inspect failure instead of silently treating nil like no warning.
- testSocketFailureStillFallsBackToCLI skips without a real local socket and uses process-global backoff. Inject endpoint selection and a per-test backoff/clock so socket failure, backoff, CLI failure, and recovery are deterministic tests on clean CI runners.
- Add fake-runner tests asserting zero stop invocations for identity/context/impact changes or cancellation; verify exactly one invocation for a confirmed unchanged target. Test failed scans and port reuse after stop.
I added the following minimal regression test file locally. These Swift assertions are UNEXECUTED here; source inspection predicts they expose current parser defects. Included in full so they can be applied directly without access to this review workspace:
import Foundation
import XCTest
@testable import LeftOpenCore
/// Safety requirements found during PR #6 review. Not yet executed: review host has no Swift.
/// These assertions intentionally expose gaps in the current implementation.
final class ContainerSafetyRegressionTests: XCTestCase {
func testEngineUDPMappingCannotOwnTCPListener() throws {
let json = #"[{"Id":"aaaaaaaaaaaa","Names":["/udp-only"],"Image":"dns","Ports":[{"IP":"0.0.0.0","PublicPort":5353,"Type":"udp"}]}]"#
let result = try XCTUnwrap(ContainerResolver.parseEngineList(Data(json.utf8), ports: [5353]))
XCTAssertNil(result[5353], "Scanner observes TCP listeners, so UDP is not ownership evidence")
}
func testCLIMappingIgnoresUDPAndChoosesTCP() {
let output = "udp-only\tdns\t0.0.0.0:5353->53/udp\nweb\tnginx\t0.0.0.0:5353->80/tcp"
XCTAssertEqual(ContainerResolver.parse(output, ports: [5353])[5353]?.name, "web")
}
func testPortOnlyLookupRefusesAmbiguousBindAddresses() throws {
let json = #"[{"Id":"aaaaaaaaaaaa","Names":["/first"],"Image":"nginx","Ports":[{"IP":"127.0.0.1","PublicPort":8080,"Type":"tcp"}]},{"Id":"bbbbbbbbbbbb","Names":["/second"],"Image":"nginx","Ports":[{"IP":"127.0.0.2","PublicPort":8080,"Type":"tcp"}]}]"#
let result = try XCTUnwrap(ContainerResolver.parseEngineList(Data(json.utf8), ports: [8080]))
XCTAssertNil(result[8080], "Without a listener address, choosing the first container is unsafe")
}
}Please address the inline blockers, retain current main's safety/Portless behavior, and run the complete macOS CI on the resulting revision. No merge performed.
| guard let docker = dockerExecutable() else { | ||
| throw CloseError(L("The `docker` command is not available on this Mac.", "这台 Mac 上没有 `docker` 命令。")) | ||
| } | ||
| return try CommandRunner.output(docker, ["stop", container.name], timeout: 20) |
There was a problem hiding this comment.
[P1] Pin the daemon and immutable container identity through discovery, inspection, review and stop. daemonSocket() independently picks a local socket, while this unqualified CLI command uses the current Docker context/environment. With a local socket plus a remote/other active context, discovery can show daemon A's container and stop daemon B's same-named container. CLI fallback can also label a local listener using a remote daemon's coincidentally matching port. ContainerInfo retains no endpoint, and CLI parsing retains no ID. Resolve a verified local endpoint associated with the forwarder, carry it plus the full container ID, and pass an explicit endpoint/context to inspect/stop. Refuse unresolved or remote ownership. A name is also mutable: if the reviewed container is removed/renamed and its name reused, this stops the replacement. Reinspect the pinned ID and reviewed impact immediately before execution; refuse stale plans and stop by ID.
| ports.contains(publicPort), resolved[publicPort] == nil else { continue } | ||
| resolved[publicPort] = info | ||
| } | ||
| } |
There was a problem hiding this comment.
[P1] Do not resolve TCP listener ownership by bare port number / first match. Scanner scans -iTCP -sTCP:LISTEN, but this loop ignores Type and IP; the CLI parser likewise accepts /udp and ignores bind addresses. UDP container A and TCP container B can both publish 5353; whichever appears first is selected, so the UI can stop A when the user selected B's TCP listener. Different bind addresses can also share a port. Match protocol and normalized local bind address on the verified daemon, and refuse ambiguous ownership rather than selecting the first entry. Regression tests for UDP/TCP and address ambiguity are in the review body.
| let containers = ContainerResolver.containers(for: forwarded) | ||
| guard !containers.isEmpty else { return activities } | ||
| return activities.map { activity in | ||
| guard let container = containers[activity.listener.port] else { return activity } |
There was a problem hiding this comment.
[P1] Reapply the runtime/endpoint ownership guard when assigning results. The query set is filtered to runtime activities, but this map applies its result to EVERY activity with the same numeric port. A native TCP process bound to another local address on the same port becomes labeled as the container and gains Stop Container for an unrelated target. Only attach metadata to a verified forwarder with a matching endpoint, bind address and protocol; add a mixed runtime/native same-port regression.
| pendingContainerStop = nil | ||
| selectedActivityID = nil | ||
| await refresh() | ||
| let stillForwarded = snapshot.activities.filter { containerPorts.contains($0.listener.port) } |
There was a problem hiding this comment.
[P1] Freeze reviewed ports and require a successful fresh scan before reporting success. containerPorts is recomputed from the mutable snapshot at confirmation; if periodic resolution loses the container (or its name is replaced), this set can be empty. stillForwarded is then empty even if the originally selected port is occupied, and the UI says that port is free. In addition, refresh() catches scan errors internally; this caller cannot know verification failed and overwrites its error notice with a stop outcome. Store the reviewed identity/impact in ContainerStopPlan (always include the selected endpoint), revalidate before stopping, and return an explicit fresh-scan success/failure after stop. A failed scan must report 'stopped, verification unavailable', and a reused port must remain a warning. Cover disappearing metadata, empty sets, scan failure, and new ownership in tests.
Publish the reviewed PR SonghaiFan#6 safety fix from original cloud commit 32b4734fe2ff1f7154eca3066e3b484bb90f4b8b using the authenticated GitHub connector. The complete source tree and ordered parents are identical to that original commit; publication metadata differs. Original source tree: d46de6a PR head parent: 0ff5d5c Integrated main parent: 633db6b Pin local Docker daemon and immutable container identity; match TCP listeners by bind address; attach metadata only to verified forwarders; freeze reviewed stop impact and require a successful fresh port scan. Linux checks passed as documented in the delivery report. Swift/macOS compilation, app tests, and real runtime smoke tests remain unrun.
This branch has not been deployed
2 participants
Motivation
Ports forwarded by container runtimes used to scatter across Apps (Docker Desktop, OrbStack) and Dev Servers (colima), named after the forwarder process (
OrbStack Helper,com.docker.backend). That hid the actual owner: you see a runtime, not the container — and the group's close hint was wrong too, because signalling the forwarder only breaks the port mapping while the container keeps running.Since the group is how a port is closed for good, containers deserve their own group with their own close path:
docker stop.What it does
docker runwithout--nameis listed by its image (a generatednostalgic_turingsays less thanredisdoes);config_fileslabel, falling back toworking_dir) or a bind-mounted host directory — no more opening Docker.app/OrbStack.app as "the folder".docker stopcommand, image, compose project, every port that closes with it, and a note when--restart=alwayswill resurrect the container on daemon restart (docker update --restart=nohint). Execution goes through the docker CLI and the result is verified by a rescan. Signalling the forwarder itself stays refused, exactly like launchd KeepAlive jobs.How resolution works
ContainerResolverfirst verifies the runtime forwarder and its local Unix socket, then tries the Engine API on that endpoint (/infoand/containers/json). If the socket request fails, the CLI fallback uses the same explicit--hostendpoint with full container IDs andcontainer inspect, not the active Docker context or inheritedDOCKER_*overrides. The canonical socket identity, daemon ID and full immutable container ID stay pinned through discovery, review, reinspection and stop. Only matching TCP bind addresses establish ownership; ambiguous or unverified ownership stays a runtime-only row without Stop Container. Foundation has no unix-socket transport, so a smallNWConnectionHTTP client handles it and decodes chunked responses. Socket and CLI failures back off separately for 60 s per endpoint, and a socket failure never disables the CLI fallback. No new macOS permissions are involved: the scanner already uses the same process-spawn and socket semantics.PortCategory's doc comment said the category "never" rests on program-name lists; container runtimes are now a documented exception, because the forwarder is the port holder — the row should name the runtime (or the container), never the mapping process.Verification
a6954e3, macOSswift test: 111/111 passed, including 22 new deterministic container safety tests covering pinned identity, TCP/bind matching, metadata assignment, fallback/backoff, stale-plan refusal, cancellation and post-stop verification.docker rundemo, the runtime's own ports) and via the CLI; the full scan was ~0.28 s with containers resolved. That timing has not been revalidated for this revision.a6954e3was triggered but reportsaction_required, with 0 jobs executed. The macOS CI matrix has not passed yet.Closes nothing; opened after testing the debug build on macOS 15 with OrbStack.