Repository navigation
feat(portforward): support multiple port mappings per forwarding rule - #3695
AkaCoder404 wants to merge 6 commits into
Conversation
One forwarding rule can now carry several local->destination port mappings, edited as explicit "Add mapping" rows that share the rule's type, bind address, and destination host. Implements the design discussed in binaricat#3671. The bridge starts one tunnel per mapping, all sharing the rule id and the pooled SSH transport, so per-mapping failure isolation comes from the existing tunnel-scoped teardown: an occupied port fails only its own mapping (OpenSSH ExitOnForwardFailure=no semantics) while the others keep forwarding. Aggregate rule status gains `partial`, computed in the domain from per-mapping states; stop, reconnect, and snapshot sync/reconcile are mapping-aware, with the bounded reconnect reusing live mapping tunnels so healthy listeners are never rebound. Single-mapping rules are unchanged: `mappings` is persisted only when more than one row exists, the first row is always mirrored into the scalar localPort/remotePort fields, and the single-mapping bridge result shape is byte-identical to before. Also: mapping rows in the new/edit form panels, per-mapping status in the rule card tooltip, i18n for en/es/ru/zh-CN/zh-TW, and an optional JSON `mappings` parameter on the portforward.rules.create/update capability tools.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a868bf0d63
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| | 'inactive' | ||
| | 'connecting' | ||
| | 'active' | ||
| | 'partial' |
There was a problem hiding this comment.
When one mapping is active and another fails, this new partial value reaches existing consumers that were not updated: isPortForwardingRuntimeBusy/isPortForwardingRuleStoppable do not classify it as running, buildAITerminalSessionInfo filters it out, and tray status falls through to Inactive. This makes Stop All skip the normal per-rule stop/reporting path and hides a live forward from AI/tray even though sockets remain open; update all active/stoppable predicates and projections to handle partial.
AGENTS.md reference: AGENTS.md:L101-L101
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Mostly addressed in f4d5430 — isPortForwardingRuntimeBusy / isPortForwardingRuleStoppable treat partial as a live runtime, buildAITerminalSessionInfo includes it, and Stop All / RuleCard handle it. One display gap remains: the tray panel has no tray.status.partial label (falls back to the raw key) and shows a neutral dot for partial rules — keeping this thread open until that lands.
…nd UI Address PR review findings: - Schedule reconnects when a mapping fails or stops while siblings remain (aggregate partial), mirroring the partial-start path instead of leaving the mapping permanently absent. - Group snapshot records by mappingId rather than record count, and keep configured-but-missing mappings as error entries, so a one-record snapshot after a renderer reload projects the rule as partial and retries the missing mapping. - Validate edit-panel mapping rows with the same checks as the create form and disable Save until they pass, so blank or duplicated ports cannot persist undefined mappings. - Treat partial as a live runtime in the bulk stop predicates, AI terminal session info, the rule card, and the tray menu, so partial rules stay stoppable and visible.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4d5430444
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Continue starting later mappings when a remote forward's tcpip-forward request is rejected (ssh2 reports "Unable to bind to ..."); only local EADDR*/listen errors were classified as per-mapping before, so one unavailable remote listen suppressed the rest. - Report status: active with fresh per-mapping start results; the active event fires before renderers install per-mapping listeners, so fresh rules otherwise reported aggregate connecting indefinitely. - Reset the reconnect attempt counter when a multi-mapping (re)start fully succeeds, matching the single-tunnel success path; a rule that recovered on its last retry previously got no budget for the next unrelated disconnect. - Preserve mapping rows through the wizard: its save now converts carried-over rows into persisted mappings (row 0 folded from the wizard's scalar ports), gates the final step on row validity, and the wizard-to-form transition folds scalars back into row 0.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c7d0cb797
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address third round of PR review findings: - Keep a disappeared mapping as a failed placeholder (empty tunnelId) while a sibling stays live — in the live status handler, the synced handler, and the reconcile heartbeat — so a rule with auto-reconnect off keeps reporting partial and naming the failed row instead of silently projecting fully active. The full-teardown path still runs once no live mapping remains. - Release a grouped connection's per-mapping listeners when a retry replaces it with the temporary connecting connection; reused bridge tunnels keep emitting, so each retry previously leaked one renderer callback per mapping and fanned events out to stale handlers. - A scalar-only AI/RPC update to a multi-mapping rule now patches the first mapping row (re-validated for duplicate local ports) instead of silently returning success with unchanged ports; the connection-change detection then restarts the tunnel. - AI terminal session info flattens multi-mapping rules into one activePortForwards entry per mapping, so every live listener is visible to getContext and the Catty prompt; dynamic rules keep the scalar entry.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3a745f51e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Stop a running rule before saving connection-changing mapping edits so the runtime no longer keeps serving removed listeners while the card shows the new rows (mirrors the agent update path and rule deletion). - Include mappings in sanitized agent rule responses so AI/RPC callers can discover and round-trip multi-mapping rules. - Adopt a recovered mapping tunnel (and its subscription) into a lost placeholder during reconciliation, so loss detection keeps seeing a mapping that another window's retry revived. - Classify remote "Unable to bind" failures as mapping-local in the shared-transport fallback, avoiding a pointless dedicated SSH redial that could re-prompt MFA.
Resolve the portForwardingBridge dynamic-SOCKS result conflict by keeping both sides: our `status: "active"` (per-mapping listeners miss the earlier event) and main's `localPort: assignedPort` (dynamic rules may bind port 0).
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
|
感谢同学提的pr，这个功能是很实用的，但是看到目前的方案是有一个新的mapping字段替代了老字段，是先前我在issue里留言提出的方案。 |
刚才验证了下 仍然是有兼容性问题，我可能要想一个过渡的方案。这个pr先保留，可能要经过几个版本，完成一些兼容性相关的工作以后才能合并 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e396b28ffd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| success: allSucceeded, | ||
| ...(firstFailure ? { error: firstFailure } : {}), | ||
| ...(cancelled ? { cancelled: true } : {}), | ||
| ...(first.reused && allSucceeded ? { reused: true } : {}), | ||
| mappings: mappingResults, |
There was a problem hiding this comment.
When the app uses terminalWorkerManager, this result is incompatible with the existing tracker in registerHandlers: the wrapper records only result.tunnelId and only when top-level success !== false (electron/bridges/portForwardingBridge.cjs:2153-2160), while a partial start returns false here and even a full start exposes only the first mapping through that scalar. onWorkerExit then notifies only tracked IDs (2234-2249), so after a worker crash a partial rule receives no failure event and a fully-started rule updates only its first mapping, leaving dead sibling listeners reported active/partial and potentially skipping recovery; track every successful result.mappings[].tunnelId in the proxy.
Useful? React with 👍 / 👎.
| const rows = draftRule.mappingRows ?? []; | ||
| if (rows.length <= 1) return rows; |
There was a problem hiding this comment.
When a user fills one mapping in form mode, opens the wizard, and changes either port there, draftRule.mappingRows still contains the original one-row value; this branch returns it unchanged, and saveRule later prefers that stale firstRow over the wizard's scalar fields. The wizard therefore silently saves the old port for this common one-row carry-over case, so row 0 needs to be folded from the scalar fields whenever a carried row exists, not only when there are multiple rows.
Useful? React with 👍 / 👎.
| const parsedMappings = normalizeRuleMappings( | ||
| type, | ||
| source.mappings !== undefined ? source.mappings : existing?.mappings, | ||
| ); |
There was a problem hiding this comment.
For a first-party portforward.rules.update that changes an existing multi-mapping local/remote rule to dynamic without also supplying the optional mappings field, this reuses existing.mappings and normalizeRuleMappings rejects the nonempty list for the new type. A type-only transition therefore fails for multi-mapping rules even though the equivalent single-mapping update succeeds; discard inherited mappings when the effective type becomes dynamic instead of requiring callers to know to send mappings: [].
Useful? React with 👍 / 👎.
| return mappings.map((mapping) => ({ | ||
| ruleId: rule.id, | ||
| label: rule.label, | ||
| type: rule.type, | ||
| localPort: mapping.localPort, |
There was a problem hiding this comment.
Fresh evidence in the current fix is that a partial rule, which necessarily contains at least one non-active mapping, is flattened from the full configured getRuleMappings(rule) list, so an occupied/failed port is emitted in activePortForwards alongside the live port with the same rule-level partial status. This makes a nonexistent listener appear in both getContext and the Catty system prompt; use per-mapping runtime status to filter the rows, or represent the partial rule without claiming each configured port is live.
AGENTS.md reference: AGENTS.md:L101-L101
Useful? React with 👍 / 👎.
Summary
Adds support for multiple port mappings in a single port forwarding rule, as designed with the maintainer in #3671: explicit "Add mapping" rows (local → destination port) sharing the rule's type, bind address, and destination host, with one-row remaining the default. A failed or occupied mapping no longer affects its siblings; status and errors are shown per mapping.
Closes #3671
Type of Change
Changes Made
PortForwardingMappingtype and optionalmappingsonPortForwardingRule, validated by the agent/CLI ops (unique local ports per rule, dynamic rejected).mappingsis persisted only when more than one row exists; the first row is always mirrored into the scalarlocalPort/remotePortfields so single-mapping rules and older stored rules are byte-identical to before.portForwardingBridge.cjs): one tunnel per mapping, all sharingruleId, type, bind address, and remote host; the existing transport pool shares one SSH connection across them. Per-mapping start dedup;mappingId/port pair added to status and runtime payloads. Single-mapping requests keep the exact legacy result shape.portForwardingService.ts): per-mapping runtime records with an aggregate rule status (newpartial, computed in the domain from per-mapping states). Stop/reconnect/snapshot-sync/reconcile are mapping-aware; the bounded reconnect restarts only failed mappings because the bridge reuses live mapping tunnels, so healthy listeners are never rebound.pf.form.mapping*/pf.rule.summary.mappingkeys in en, es, ru, zh-CN, zh-TW.mappingsparameter onportforward.rules.create/update; regenerated capability tool specs.Screenshots / Demo
New Forwarding form with "Add mapping" rows (local/remote types); a rule with several mappings shows one summary line per mapping and per-mapping status in its tooltip.
Testing
npm run dev)npm run lint)npm test)npm run generate:capability-tools)Notes:
ssh2server (the loopback method from [Feature] Support multiple ports in a single forwarding rule #3671): two mappings forwarded traffic over one SSH connection; an occupied third port failed per-mapping withEADDRINUSEwhile the siblings kept forwarding; stop released every listener.npm test: 13,420 pass. The two failures (transferBridge.test.cjs,grokAcpDriverauth-method precedence) fail identically onmainand are unrelated.singleChannelSshhosts each mapping dials its own dedicated SSH session, matching today's behavior for separate rules; noted as a known limitation rather than a behavior change.mappingssurvives save/reopen and Gist round-trips; older app versions persist it as opaque data but would drop it if they edit such a rule with their scalar-only form.Checklist