fix(psmux): refuse an attached-client count that names another session - #714
Open
dracic wants to merge 2 commits into
Open
fix(psmux): refuse an attached-client count that names another session#714dracic wants to merge 2 commits into
dracic wants to merge 2 commits into
Conversation
`_attached_clients` read `#{session_attached}` with a plain `-t <session>` and
trusted whatever integer came back. A `-t` read resolves through that name's
port AND key files, and psmux does not check that the server behind them owns
the name, so a duplicated registry entry answers at exit 0 with a real count
belonging to a session nobody asked about. Both files are needed — the server
rejects a key mismatch before running anything (`server/connection.rs:354`) —
so a merely recycled port fails rather than answering.
Since bmad-code-org#659 that count also gates a targeted `switch-client`, in the NONZERO
direction: a borrowed count vouches for a switch that moved nobody, which
`return_attached_client` turns into `RETURNED` and a cleared return option for
a human still sitting where they were.
The probe now reads `#{session_attached}|#{session_name}` in one round trip and
returns the count only when the answered name is the session that was asked
for. The count comes first so a single `partition` stays unambiguous. An empty
or `:`-bearing session name is refused before the spawn — the bmad-code-org#221 rule the
module's other target-composing call sites already applied.
What this does NOT close, now stated where it was overclaimed: a foreign server
whose session carries the same name, or one differing only in whitespace the
seam's own `.strip()` merges, is not separable by a name. That stays bmad-code-org#531.
The prose is corrected alongside, because it named a mechanism measurement
refutes: a server that is merely *gone* fails closed at rc 1, and the seam's
`={session}` token is a no-op here since `parse_target` strips the `=` before
routing.
Tests: the client-verb harness moves to the combined format with no expected
value changed; new rows cover the differently-named answer, an unnamed one, a
whitespace-only difference, a dead server, `|` inside a name, and the two
pre-spawn refusals. A live premise probe pins that a port and key pair is still
answered by whichever server it names, and says which of its two assertions
means the guard is redundant versus inert.
Closes bmad-code-org#671
|
Warning Review limit reachedNext included review available in 26 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@codex review |
![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: 9966f4a650
ℹ️ 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".
…ister Twelve lines of implementation narrative — probe mechanics, the bmad-code-org#659 history, the residual edge cases — where the convention asks for terse, scannable and imperative, and where every neighbouring entry in the section is four lines of "do X, so Y can no longer Z (#N)". The user-visible facts survive: what is refused, what it prevented, and the one scope limit worth a reader's time.
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.
What
PsmuxMultiplexer._attached_clientsnow reads#{session_attached}|#{session_name}in one round trip and returns the count only when the answered name is the session that was asked for. An empty or:-bearing session name is refused before the spawn.Why
A
-t <session>read resolves through that name's port and key files, and psmux does not check that the server behind them owns the name — so a duplicated registry entry answers at exit 0 with a real count belonging to a session nobody asked about.Since #659 that count also gates a targeted
switch-client, in the nonzero direction: a borrowed count vouches for a switch that moved nobody, whichreturn_attached_clientturns intoRETURNEDand a cleared return option for a human still sitting where they were.Fixes #671
How
partitionstays unambiguous when a name contains the separator.:-bearing name before spawning — the Parked-window return target is a bare pane id — unresolvable cross-session on psmux (one server per session) #221 rule this module's other target-composing call sites already applied (current_return_target).={session}token is a no-op here sinceparse_targetstrips the=before routing, and a recycled port alone cannot reach the hazard because the server rejects a key mismatch first (server/connection.rs:354).What this deliberately does not close
A foreign server whose session carries the same name — or one differing only in surrounding whitespace, which
current_session's own.strip()merges before the compare ever runs — is not separable by a name. That is #531's subject, and the CHANGELOG, the docstrings and the live probe's failure messages all say so rather than implying the guard is total.Testing
209 passed across
tests/test_psmux_backend.pyand the live gate, on Windows against the installed psmux 3.3.8; the live gate consumes zero LLM tokens and touches only an isolatedPSMUX_DATA_DIRregistry.trunk checkclean;pyrightshows only the five pre-existing Windows-onlyplatform_util.pyerrors.The client-verb harness moves to the combined format with no expected value changed in any of the 25 existing tests. New rows cover a differently-named answer, an unnamed one, a whitespace-only difference, a dead server,
|inside a name, and the two pre-spawn refusals. Every negative assertion was ablated — deleting the identity compare fails five tests, deleting the:/empty guard fails two, and deleting the rc guard fails the dead-server test (whose stdout is a well-formed matching answer on purpose, so it pins the guard rather than passing for its own reasons).A new
test_premise_*probe pins that a port and key pair is still answered by whichever server it names, and distinguishes its two red directions: a red on the rc means the guard became redundant, a red on the answered name means it became inert and needs replacing rather than removing.Changelog
Added under
## [Unreleased]→### Fixed.