refactor(cli): Share the signal router instead of borrowing it - #963
Open
JeanMertz wants to merge 9 commits into
Open
refactor(cli): Share the signal router instead of borrowing it#963JeanMertz wants to merge 9 commits into
JeanMertz wants to merge 9 commits into
Conversation
A command plugin cannot read from the terminal: its stdin carries the
host protocol, so anything typed there would be parsed as a message.
It also has no way to know which editor the user has configured. Both
of those live on the host side, which is why composition is a request
the plugin makes rather than something it does itself.
`compose` asks for one of four things, and `composed` carries the
answer back: a single line with an optional default, a multi-line
buffer with `Ctrl+X` to escape into the configured editor, one choice
from a list, or any number of choices from a list. A cancelled prompt
and a run with no terminal produce the same answer, an empty one, so a
plugin has one path to handle rather than two.
`jp ticket` is the first consumer. Omitting what a subcommand needs
asks for it rather than failing:
jp ticket add # kind, then title and description
jp ticket comment T0001 # picks the ticket, then the body
jp ticket import # multi-select over the open issues
`PROTOCOL_VERSION` is 2. There is no negotiation yet, so a plugin built
against 2 and run against an older `jp` sends a `compose` the host
cannot parse and then waits for a reply that never comes.
`ReadyMessage` and a version handshake land separately.
Signed-off-by: Jean Mertz <git@jeanmertz.com>
A plugin and the `jp` it runs under are installed separately and can drift apart. The failure that follows is the worst shape available: the plugin sends a message the host cannot parse, the host ignores it, and the plugin blocks forever on a reply that will never come. Nothing reports anything, and the run hangs. `ready` carries the lowest protocol version the plugin can work with, so both ends check at the one point where checking still helps. A plugin calls `jp_plugin::ready` with what it needs and the version from `init`, and gets back either the message to send or the `exit` to send instead. A host seeing a plugin that needs more than it speaks stops with an error naming both numbers. The field is optional on the wire, defaulting to 1, so a plugin built before it existed still parses and is taken at its word. Nothing needs reinstalling to keep working. `PROTOCOL_VERSION` stays at 2: this adds a handshake, not a message. `jp-path` and `jp-serve-web` require 1, since they only read what the first version already carried. `jp-ticket` requires 2, for `compose`. Signed-off-by: Jean Mertz <git@jeanmertz.com>
`run_plugin` spawned the process, wired three pipes, started two threads, and ran the protocol, in one body. `message_loop` held the reader loop and every request handler in a single match. Both are about to grow: turn delegation adds handlers that block for minutes, which needs the loop and the request handling to come apart. `spawn_plugin` returns a `PluginProcess` carrying the child and its pipes. `stop_plugin` sends `shutdown` and escalates to a kill if the grace period runs out. `handle_request` answers one message and reports through `Flow` whether the loop carries on, which leaves `message_loop` as reading, dispatching, and the two ways a run can end. No behaviour changes. `handle_request` being callable on its own is the point, and the two tests it gains cover the split between a clean exit stopping the loop and a failing one surfacing its code and reason. Signed-off-by: Jean Mertz <git@jeanmertz.com>
A plugin could read the conversation list and a conversation's events, but not change anything about one. Anything acting on what it read had to tell the user to go run `jp conversation` themselves. `archive_conversation` and `set_title` are the two mutations that need no turn behind them. Both take the conversation's lock, so they cannot land while a turn is running: archiving moves files, and renaming writes metadata a turn is also holding. A conversation someone else is working on comes back as a refusal rather than waiting. Both answer with `done`, which carries only the correlation ID, and a failure comes back as `error` naming which request it belongs to, so a plugin with several in flight can tell them apart. An absent or blank title clears the name rather than storing an empty string, leaving the conversation eligible for a generated title again. `run_plugin` took eleven arguments once the workspace had to be mutable and the session threaded through. Six of them were fields of `Ctx`, so it takes `&mut Ctx` and reads them itself, including building the `Composer` that used to be handed in. Signed-off-by: Jean Mertz <git@jeanmertz.com>
`list_conversations` handed a plugin bare deciseconds, `17000000000`. That is the internal representation, not the name a conversation has anywhere else: JP prints `jp-c17000000000`, and that is what a user types. A plugin displaying what it was given showed a number nobody could paste back into `jp`. The host emits the canonical spelling and accepts either, so a request naming a conversation the old way still resolves. `parse_conversation_id` is the one place that decides, which `read_events` and the conversation mutations both go through. Version negotiation cannot express this: a plugin declaring a protocol version says nothing about which spelling it wants, so there is no way to serve both generations of plugin. It is safe now only because the sole consumer is `jp-serve-web`, in this repository, which feeds ids straight back to `read_events` and so survives on the parser accepting both. Doing it later means doing it to `query`, `interrupt`, the draft requests, and `created` as well. Signed-off-by: Jean Mertz <git@jeanmertz.com>
A conversation's query draft is the half-written message `jp query` seeds an editor from and saves on interrupt. A plugin offering somewhere to compose had no way to see it or add to it, so text typed in one place was invisible in the other. `read_draft` returns the draft and a fingerprint of it. `write_draft` carries that fingerprint back, and a write based on a version the draft has since moved past is refused rather than applied: the other writer's text is precisely what the caller has not seen, and losing it is what this guards against. The refusal answers with what is on disk, so the caller can show both. The fingerprint hashes content rather than reading a modification time, so rewriting a draft with identical text is not mistaken for someone else's edit. An empty write removes the draft instead of leaving a blank file, which the CLI would otherwise seed an editor from and treat as a recovery copy. Drafts stay in user-local storage and are never projected into the workspace tree: a half-written message is not something a teammate should end up with. A workspace without user-local storage has nowhere to put one, and says so rather than falling back to the shared tree. Signed-off-by: Jean Mertz <git@jeanmertz.com>
`--cfg skill/rfd` resolves a name against the config load paths, and nothing could ask what names exist. A plugin offering a choice of configurations had to hardcode a list or walk the tree itself, in both cases guessing at rules `jp_config` already owns. `list_configs_in_load_path` is the inverse of `find_file_in_load_path`: it walks a load path and reports every configuration file as the segment that selects it, the relative path without its extension. Directories are part of the segment, so `skill/rfd.toml` is `skill/rfd`, which is what `--cfg` takes. An absent load path holds nothing rather than failing, since a workspace need not have every directory the load path names. `list_configs` answers over the protocol from the same three roots `--cfg` searches: the user's global config directory, the workspace, and user-local storage. Roots are searched independently and the results merged into one sorted set, because a segment present in more than one is still one selectable thing: naming it merges all of them. Each entry carries its namespace and name split out, so a caller can group by directory without parsing the segment. Signed-off-by: Jean Mertz <git@jeanmertz.com>
`Query::run` gathered what a turn needs and ran it in one stretch, with `handle_turn` taking fourteen arguments off the end of it. Everything in between read `ctx`, so a turn could only be driven by the command that owned the CLI context. `TurnInputs::collect` reads the context once and `TurnInputs::run` does not touch it, which splits the fast half from the slow half. Collecting resolves attachments and records where the turn came from; running waits on MCP servers, resolves tools, assembles the thread, and calls the turn. `handle_turn` becomes `Query::run_turn`, an associated function, since it never used `self`. Attachment loading moves ahead of the MCP startup wait as a result. An attachment that shells out or fetches now does so before the startup timer rather than after, and a failing one is reported earlier. Nothing else changes order: the thread is still built before the stream is sanitized, and `run_turn`'s body is untouched. `get_config_delta_from_cli` becomes `pub(crate)`, so a second caller computes the same difference rather than its own. The router is borrowed rather than owned, because it holds the process-wide signal task and cannot be cloned. That is what keeps `TurnInputs` tied to the lifetime of the context it came from; freeing it to move to another task means making the router shareable, which belongs with the signal work. Signed-off-by: Jean Mertz <git@jeanmertz.com>
`SignalRouter` held its signal task's `JoinHandle` directly, which made the struct unclonable, which made every holder a borrower. That was fine while the only holder was the command that created it. A turn is about to be startable by something other than the `query` command, and it has to run away from the thread that started it. It still needs to register an interrupt handler, and that handler is only worth anything if the signal task can reach it — so the router has to be shareable rather than borrowed, and sharing has to mean one router with several handles, not several routers. The handle moves behind an `Arc`, since there is one task however many references to the router exist, and `Clone` is derived. Nothing about delivery changes: every clone reads the same handler stack, the same shutdown token, and the same escalation counter. `TurnInputs` owns the router as a result, and so borrows nothing from the context it was collected from. Signed-off-by: Jean Mertz <git@jeanmertz.com>
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.
SignalRouterheld its signal task'sJoinHandledirectly, which made the struct unclonable, which made every holder a borrower. That was fine while the only holder was the command that created it.A turn is about to be startable by something other than the
querycommand, and it has to run away from the thread that started it. It still needs to register an interrupt handler, and that handler is only worth anything if the signal task can reach it — so the router has to be shareable rather than borrowed, and sharing has to mean one router with several handles, not several routers.The handle moves behind an
Arc, since there is one task however many references to the router exist, andCloneis derived. Nothing about delivery changes: every clone reads the same handler stack, the same shutdown token, and the same escalation counter.TurnInputsowns the router as a result, and so borrows nothing from the context it was collected from.