fix: finish Rust port-base migration (17000 → 22000) across remaining call sites#47
Closed
gavin5011 wants to merge 7 commits into
Closed
fix: finish Rust port-base migration (17000 → 22000) across remaining call sites#47gavin5011 wants to merge 7 commits into
gavin5011 wants to merge 7 commits into
Conversation
Commit 89168a3 removed OPENSESSIONS_RUST opt-in and pinned server-common.sh to PORT_BASE=22000, but left two callers on the legacy 17000 base: - apps/tui-rs/src/runtime_config.rs:20 hardcoded 17_000 + key - packages/runtime-rs/src/shared.rs::resolve_server_settings still branched on OPENSESSIONS_RUST=1 and defaulted to 17_000 (also resolve_server_port had 17_000 as base default) Result: the TUI sidebar binary connects to ws://localhost:(17000+key) while the server listens on (22000+key), so spawned sidebar panes die immediately with 'Connection refused' and the user sees nothing on prefix+o>s. This commit aligns all three call sites on 22000: - TUI runtime_config.rs: 22_000 + key - runtime-rs shared.rs resolve_server_port default: 22_000 - runtime-rs shared.rs resolve_server_settings: drop OPENSESSIONS_RUST branch, pin base = 22_000 Tests in packages/runtime-rs/tests/config_and_shared.rs still hold the old 17000-derived port assertions and need to be updated in a follow-up before this can land upstream.
Commit 4b862c4 finished the migration for the Rust server/TUI runtime helpers, but the rest of the tree was still on the legacy 17000 base. Codex+Claude branch-review surfaced the gaps: - TS integrations (Amp / Pi extension) compute the port from 17000+server_key and post agent events to the wrong port now that the server binds 22000+server_key. - Test assertions in apps/tui-rs/tests/{cli,protocol}.rs and packages/runtime-rs/tests/config_and_shared.rs still expect 36916 / 25011, which only hold with the old base. - The 'OPENSESSIONS_RUST coexist with TS bun' doc comment on resolve_server_port_with_base no longer matches the implementation (no env-var branch left). - 22_000 was duplicated as a magic number across resolve_server_port and resolve_server_settings without a SSoT constant. - apps/tui-rs/src/runtime_config.rs computed '22_000 + key' in u16, losing the 5000-wide overflow margin that 17_000 used to provide; a server key >= 43_536 would debug-panic / silently wrap. - docs/ratatui-migration/{04-protocol-and-types,11-config-and-persistence}.md still showed 17000-based examples. Changes: - integrations/amp/opensessions.ts: 17000 -> 22000 (both call sites) - integrations/pi-extension/opensessions-runtime.ts: same - packages/runtime-rs/src/shared.rs: introduce pub const SERVER_PORT_BASE: u32 = 22_000 and use it from resolve_server_port + resolve_server_settings; rewrite the doc comment for resolve_server_port_with_base to drop the dead OPENSESSIONS_RUST + TS bun coexist narrative and document the base+server_key <= u16::MAX precondition instead. - apps/tui-rs/src/runtime_config.rs: mirror the SERVER_PORT_BASE constant and compute (SERVER_PORT_BASE + key as u32) as u16 so the arithmetic stays in u32 and never debug-panics. - apps/tui-rs/tests/cli.rs, apps/tui-rs/tests/protocol.rs, packages/runtime-rs/tests/config_and_shared.rs: update assertions (36916 -> 41916, 25011 -> 30011) and refresh the test comment block that still described the dead TS bun coexist setup. - docs/ratatui-migration/{04,11}: 17000 -> 22000 in examples. cargo test --release: all 14 test binaries pass.
…aming Branch-review round 2 surfaced eight remaining issues on the diff introduced by commits 4b862c4 + 85b36e4. This patch resolves the correctness ones inline and renames the now-misleading tests. Correctness (F2/F3 — Amp + Pi TS integrations): Both resolvers parsed OPENSESSIONS_SERVER_KEY without NaN guard or bounds check, so a non-numeric value yielded `http://127.0.0.1:NaN` and a key >= 43536 produced a port > 65535. The previous 17000 base hid the upper bound at 48535; the migration tightened it to 43535 without adding the check. Validate parse + bounds and fall through to the tmux-hash branch / DEFAULT_SERVER_PORT otherwise. Correctness (F4/F5 — Rust port arithmetic): Both `apps/tui-rs/src/runtime_config.rs::resolve_server_port` and `packages/runtime-rs/src/shared.rs::resolve_server_port_with_base` cast `base + key` to u16 with `as u16`, which silently truncates when the sum exceeds u16::MAX. The doc comment introduced in the previous commit documented the precondition but did not enforce it; in practice the same env var that breaks the TS branches reaches this code through resolve_server_key + parse::<u32>(). Replace the unchecked cast with `u16::try_from(...).ok().unwrap_or(DEFAULT_SERVER_PORT)` (plus checked_add on the shared.rs side, since base is u32 + key is u32 there) so out-of-range input falls back deterministically to the same default the parse-failure branch already uses. Testing (F1/F6/F8 — stale 'rust_base' terminology in test names): - resolve_server_settings_uses_rust_port_base_when_opensessions_rust_set → resolve_server_settings_ignores_opensessions_rust_env_var (the env var no longer affects the port; rename matches the behavior the test now actually exercises). Assertion message also updated from "OPENSESSIONS_RUST=1 must use base 22000" to "Rust stack must bind base 22000". - resolve_server_settings_keeps_ts_port_base_when_opensessions_rust_unset → resolve_server_settings_uses_default_port_base_without_opensessions_rust (TS base no longer exists; the test actually pins the default base when the env var is missing). - resolve_server_settings_explicit_port_overrides_rust_base → resolve_server_settings_explicit_port_overrides_port_base (single port base now, no 'rust' qualifier needed). Deferred to follow-up: F7 (resolve_server_port_with_base now always called with SERVER_PORT_BASE, so the `base: u32` parameter is dead configuration surface — kept public for now since downgrading visibility is an API change separate from the migration fix). cargo test --release: all 15 test binaries pass.
…r overflow paths Branch-review round 3 surfaced two more issues caused by the round-2 patch: Correctness — endpoint divergence on invalid OPENSESSIONS_SERVER_KEY: The TS Amp/Pi resolvers fell through to the TMUX-hash branch when the explicit-key parse/range check failed, while the Rust server (packages/runtime-rs/src/shared.rs::resolve_server_port_with_base) falls back to DEFAULT_SERVER_PORT on the same input. With a bad OPENSESSIONS_SERVER_KEY set, client posts would go to the TMUX port while the server listens on DEFAULT_SERVER_PORT — a silent drop. Make both TS resolvers return DEFAULT_SERVER_PORT in the same case so client and server agree. Testing — overflow fallback coverage: The round-2 try_from / checked_add changes added a deterministic fallback for keys that push 22000+key above u16::MAX, but no test exercised the fallback. Add cases at key=50_000 (Some(u16) sum 72_000 > u16::MAX) and key=u16::MAX / "4294967295" (also covers the checked_add(u32) path on the runtime-rs side). TS-side test coverage for the Amp/Pi resolvers is deferred — the integrations have no existing test infrastructure in-tree and adding it is out of scope for this migration fix; tracked as follow-up. cargo test --release: all 15+ test binaries still pass.
… merge
R4 (Codex+Claude cross-validated) surfaced two high-severity correctness
issues and four supporting items on the round-3 commit. This patch
addresses all six on the diff and leaves a documented duplication
warning where consolidation would change crate dependencies.
Correctness — F1/F2 (high):
`Number.parseInt("123abc", 10)` returns 123 in JS, not NaN, so the
Amp + Pi resolvers were silently sending events to port 22123 while
the Rust server (which uses strict `parse::<u32>()`) was listening
on DEFAULT_SERVER_PORT 7391. Same for "0.5" → JS 0 vs Rust Err.
The previous round's mirror comment was wrong about the parity it
claimed. Switch to `Number(explicitKey)` + `Number.isInteger` so
the TS path Err's on the same inputs as Rust's u32 parse.
Correctness — F4 (medium):
Both `resolve_server_port_with_base` (runtime-rs) and TUI
`resolve_server_port` silently fell back to DEFAULT_SERVER_PORT
when SERVER_PORT_BASE + key overflowed u16. Add a single
`eprintln!` on that branch identifying the inputs so operators see
why the client/server cannot agree on a port. Parse-failure stays
silent (it has a clearer cause and tends to fire on user typos).
Doc — F5 (low):
`resolve_server_port_with_base` doc still claimed "callers must
ensure base + server_key <= u16::MAX" after round 3 made the
implementation enforce a fallback. Rewrite to describe the actual
contract (fallback + stderr warn).
Testing — F6 (medium, downgrade in R4):
Round 3 renamed the test to
`resolve_server_settings_ignores_opensessions_rust_env_var` but
the body only exercised the OPENSESSIONS_RUST=1 case. The
"ignores" claim was unverified. Merge the env-set and env-unset
cases into the same test and assert their ports are equal — that
is the property the name promises. The standalone
`_uses_default_port_base_without_opensessions_rust` test is now
redundant and removed.
Testing — F7 (low):
`resolve_server_settings_explicit_port_overrides_port_base` no
longer needs to set OPENSESSIONS_RUST=1 since the env var has no
effect on the port. Drop the stale setup so the test exercises
only the property it names.
Convention — F8 (low, deferred):
`SERVER_PORT_BASE` is duplicated in apps/tui-rs/src/runtime_config.rs
and packages/runtime-rs/src/shared.rs because tui-rs does not depend
on runtime-rs. Adding the dep is broader scope than this migration
fix. Document the duplication and the in-sync invariant on both
sides instead — moving the const to a shared crate
(sidebar-protocol-rs is a natural home) is left as follow-up.
Codex also reported (F3, medium → downgrade in R4): the new TUI
overflow fallback is unreachable through the only existing call site
(hash is bounded by % 20_000). Kept the defensive try_from + eprintln
anyway — `OPENSESSIONS_SERVER_KEY` is the unbounded path that motivates
it.
cargo test --release: 211 tests pass (one redundant test removed; the
merged env-var test now asserts equality across set/unset paths).
R5 (Codex+Claude) found that the round-4 fix using `Number()` +
`Number.isInteger` was still too lenient. JS coerces `Number("1.0")`
→ 1, `Number("1e3")` → 1000, `Number("0x10")` → 16, all of which
pass `Number.isInteger`, while Rust's `parse::<u32>()` rejects them
all. The same lenience was present on the `OPENSESSIONS_PORT` branch
(`Number.parseInt` accepts trailing garbage and decimal prefixes),
which R5 surfaced as a second pair of medium findings.
Replace both `OPENSESSIONS_PORT` and `OPENSESSIONS_SERVER_KEY`
parsing with a `/^[0-9]+$/` regex gate before `Number()` in both
Amp and Pi extensions. This matches the inputs accepted by Rust's
`u32::from_str` / `u16::from_str` (decimal ASCII digits only,
leading zeros allowed, no signs / decimals / scientific / hex) so
the client and the Rust server agree on every reachable input.
Deferred (R5):
F3/F4 (medium, testing): the Amp and Pi resolveServerPort lack
any in-tree integration test coverage. Adding test infrastructure
for TS integrations is out of scope for the migration fix.
F5 (medium, correctness): claude-only false alarm — the u32
expression `SERVER_PORT_BASE + u32::from(key)` with
SERVER_PORT_BASE=22_000 and key: u16 cannot overflow u32 (max
22_000 + 65_535 = 87_535).
F8-F11 (low): cosmetic — port-ceiling re-check, stderr message
detail, const duplication (also F10 from earlier), redundant
test assertion.
cargo test --release: still passes.
…parity with Rust) R6 (Codex) flagged that the R5 regex was stricter than Rust on one input class: `u16::from_str` and `u32::from_str` accept an optional leading `+` (`+8123` parses to 8123 in Rust), but the TS regex rejected those, so a user setting OPENSESSIONS_PORT=+8123 would have the Rust server bind 8123 while the TS client fell back to TMUX-derived or DEFAULT_SERVER_PORT — exactly the silent endpoint mismatch this PR is supposed to eliminate. Note: Claude's Stage B refuted the finding based on incorrect Rust knowledge (claimed `u32::from_str` rejects `+`). Verified empirically on rustc 1.96.0: `"+8123".parse::<u16>()` returns Ok(8123), so the Codex side was correct. Applying the fix to both Amp and Pi. Loosen the regex to `/^\\+?[0-9]+$/` and document the empirical verification next to it in both files. Deferred from R6 (all CONFIRMED or REFUTED at low / medium severity): F3 (medium): u32 sum overflow on tui-rs side — false alarm (key: u16 is structurally bounded, 22_000 + 65_535 cannot overflow u32). F4, F8, F10, F11 (low): cosmetic. F5, F6 (medium): tmux hash branch overflow — hashServerKey is bounded `% 20_000` so 22_000 + 19_999 = 41_999 cannot overflow u16; the explicit-key branch already has bounds check. F9 (medium): SERVER_PORT_BASE still duplicated across two crates. The cross-reference comments added in R4 document the in-sync invariant; lifting the const into a shared crate (sidebar-protocol-rs is the natural home, but runtime-rs would need to gain a new dep on it) is out of scope for this fix. cargo test --release: still passes.
![inspect-review[bot]](https://avatars.githubusercontent.com/in/3177512?s=60&v=4){kind=small}
There was a problem hiding this comment.
inspect review
Triage: 20 entities analyzed | 0 critical, 0 high, 15 medium, 5 low
Verdict: standard_review
Findings (1)
- [low] In packages/runtime-rs/src/shared.rs, resolve_server_port_with_base has truncated error message. The string literal 'DEFAULT_SE' is incomplete and will cause a compilation error.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
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.
Summary
Commit
89168a3("chore: remove TypeScript fallback, Rust-only runtime") pinnedintegrations/tmux-plugin/scripts/server-common.shtoPORT_BASE=22000and removed theOPENSESSIONS_RUSTopt-in flag. Several call sites were missed: the TUI sidebar binary, the shared runtime helpers, the Amp/Pi TS integrations, tests, and docs all kept the legacy 17000 base.After a fresh install,
prefix + o → ssilently fails — the server binds22000 + server_key, but the spawned sidebar pane connects tows://localhost:17000 + server_key, getsConnection refused, and dies before the user sees anything.This PR finishes the migration end-to-end and hardens the env-parsing edges to keep client and server on the same port.
How I noticed
Fresh TPM install on macOS,
cargo build --releasesucceeds, butprefix + o → sproduces no visible output. Debug log shows the gap:lsofconfirms the server is on 41916 (= 22000 + 19916 for socket/private/tmp/tmux-501/default). The TUI was reading the legacy 17000 base fromapps/tui-rs/src/runtime_config.rs:20, so its WebSocket connection went to the wrong port and the pane died instantly.A repo-wide grep for
17_000/17000surfaced seven sites still on the old base.Changes
Runtime — root cause
apps/tui-rs/src/runtime_config.rs:17_000 + key→(SERVER_PORT_BASE + u32::from(key)) as u16viau16::try_from(...).unwrap_or_else(...)so out-of-range keys fall back toDEFAULT_SERVER_PORTand log to stderr.packages/runtime-rs/src/shared.rs:resolve_server_portdefault base →SERVER_PORT_BASE = 22_000;resolve_server_settingsdrops the deadOPENSESSIONS_RUSTbranch and pins the same constant;resolve_server_port_with_baseenforces the documentedbase + key <= u16::MAXprecondition withbase.checked_add(key).and_then(|sum| u16::try_from(sum).ok()).unwrap_or_else(...). The doc comment is rewritten to drop the dead "OPENSESSIONS_RUST + TS bun coexist" narrative.Integrations
integrations/amp/opensessions.tsandintegrations/pi-extension/opensessions-runtime.ts: 17000 → 22000 on both theOPENSESSIONS_SERVER_KEYandTMUX-hash branches.const DECIMAL_INTEGER_RE = /^\+?[0-9]+$/.Number.parseIntis too lenient ("123abc"→ 123,"0.5"→ 0);Number()is too permissive ("1e3","0x10"coerce to integers). The regex matches what Rust'su32::from_str/u16::from_straccept (decimal ASCII digits + optional leading+; empirically verified onrustc 1.96.0that"+8123".parse::<u16>()returnsOk(8123)).OPENSESSIONS_SERVER_KEY, fall back toDEFAULT_SERVER_PORTrather than the TMUX-derived port, mirroring the Rust server's behavior on the same input so client and server resolve to the same port.Tests
apps/tui-rs/tests/{cli,protocol}.rs: 36916 → 41916;protocol.rsadds overflow fallback cases (key=50_000andu16::MAX→DEFAULT_SERVER_PORT).packages/runtime-rs/tests/config_and_shared.rs: 36916 → 41916, 25011 → 30011; added overflow fallback cases (server_key="50000","4294967295"); renamed three tests whose names referred to the deadrust_base/ts_port_basedistinction; merged the redundant set/unset env-var pair into one test that asserts equality.Docs / comments
docs/ratatui-migration/{04-protocol-and-types,11-config-and-persistence}.md: 17000 → 22000 in examples.Test plan
cargo test --release— all test binaries pass.prefix + o → s→ sidebar panes spawn in every active window, server listens on22000 + server_key, TUI connects to the same port,/tmp/opensessions-err.logstays empty across multiple toggles.Deferred to follow-up
resolve_server_port_with_baseis now only called withSERVER_PORT_BASEin tree, sobase: u32is effectively dead configuration surface. Kept public — downgrading visibility is a separate API change.SERVER_PORT_BASEis duplicated inapps/tui-rs/src/runtime_config.rsandpackages/runtime-rs/src/shared.rsbecausetui-rsdoes not depend onruntime-rs. Cross-reference comments document the in-sync invariant; lifting the const into a shared crate (sidebar-protocol-rsis the natural home, butruntime-rswould need a new dep on it) is out of scope here.OPENSESSIONS_PORTwith values outside(0, 65535]silently falls through to theSERVER_KEY/ TMUX branches on both Rust and TS. Surfacing that as a hard fallback toDEFAULT_SERVER_PORTmatches theSERVER_KEYarm but changes existing behavior on both sides — proposed as a separate follow-up.