fix(cli): Re-read the conversation index for each plugin read - #968
Open
JeanMertz wants to merge 13 commits into
Open
fix(cli): Re-read the conversation index for each plugin read#968JeanMertz wants to merge 13 commits into
JeanMertz wants to merge 13 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>
A plugin could read a conversation and write its draft, but not send anything. Composing a message somewhere other than a terminal meant telling the user to go and run `jp query` themselves. `query` asks the host to run a turn, which is the same turn `jp query` runs: the host locks the conversation, appends the request, calls the provider, executes tools, and persists the events. A plugin doing this itself would need the user's credentials, the tool registry, and the MCP servers, and would be a second implementation of the turn loop. The turn runs under the conversation's configuration, not the host's. A host resolves its config once at startup with no conversation in view, so its persona, skills and enabled tools are whatever the bare workspace has; running a turn under that would answer with the wrong model and no tools. Any `cfg` the request names is layered on top and recorded as a config event, so the choice holds for later turns and the stream carries the reason, which is what `jp q --cfg` does. Interrupt handling is pinned to stopping. The default streaming action opens the interrupt menu, and with no keyboard attached that menu blocks on a read nobody can satisfy: the turn keeps the conversation locked, every later request is refused as already-locked, and the host's terminal sits at a prompt meant for someone who is elsewhere. Turns go to a task of their own and the message loop is now async, so reads are still answered while one runs. `query` with `new` gets two replies: `created` as soon as the conversation exists, carrying the id the caller has no other way to learn, then `query_complete` when the turn ends. Failures carry the whole error chain, because the outermost message is a category and the cause is what hangs off it. Output goes to a sink rather than the host's terminal. Nobody typed the message there, and two concurrent turns would braid into one stream with no way to tell them apart. Signed-off-by: Jean Mertz <git@jeanmertz.com>
A plugin could start a turn but not stop one. A turn that had gone wrong ran to completion holding the conversation's lock, and every later request against that conversation was refused as already-locked. `interrupt` names the conversation whose turn should stop, and reaches it the way a Ctrl-C does, so it escalates on repeat exactly as the terminal does: the first asks the turn to wrap up, pressing on abandons it. Fire-and-forget, because what the interrupt did shows up in the conversation, and the turn's outcome still arrives as the reply to its original `query`. The router grows targeted delivery to make that possible. A turn now registers its handler under its conversation, and `interrupt_scope` notifies that scope alone. A Ctrl-C keeps going to whichever handler is topmost, because "whatever I am looking at" is the right guess for a keyboard; it is the wrong guess for a request that already said which conversation it meant, and with several turns in flight it would stop an arbitrary other one. The scope is the id itself rather than a rendering of it. An id has more than one spelling, and comparing one spelling against another matches nothing while looking exactly like a stop button that does not work. A scope with no handler reports that nothing was reached rather than failing. Its turn finished, so there was nothing left to interrupt. Signed-off-by: Jean Mertz <git@jeanmertz.com>
A turn wrote its events once the streaming phase finished. A process that died partway lost everything the assistant had already said, even the parts it had finished saying, and anything reading the conversation from another process saw nothing at all until the phase completed. Each completed content block and each tool call is written as it lands. A `Flush` is the provider saying a block is final, which makes it the first point where the stream is consistent enough to persist; a tool call request is the other, since the call is a fact before its result exists. That makes a turn observable from outside the process running it, which is what lets a second frontend watch one rather than wait for it. A failed write is not fatal: the phase-end flush tries again, and the in-memory stream was already correct. Signed-off-by: Jean Mertz <git@jeanmertz.com>
A plugin host loads the conversation index once at startup and does not own the store. A `jp query` in another terminal, or another plugin, appends events to a conversation whose metadata and stream this process cached and would otherwise keep serving for its lifetime. A conversation started elsewhere never appeared at all. `list_conversations` and `read_events` re-read the index first, which drops both caches so the read that follows comes from disk. The scan is a directory listing per storage root; metadata and streams stay lazy, so only what the request actually reads is loaded again. Not a sanitize pass. That repairs a store on startup and can move broken conversations aside, which is not something a page view should do. 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.
A plugin host loads the conversation index once at startup and does not own the store. A
jp queryin another terminal, or another plugin, appends events to a conversation whose metadata and stream this process cached and would otherwise keep serving for its lifetime. A conversation started elsewhere never appeared at all.list_conversationsandread_eventsre-read the index first, which drops both caches so the read that follows comes from disk. The scan is a directory listing per storage root; metadata and streams stay lazy, so only what the request actually reads is loaded again.Not a sanitize pass. That repairs a store on startup and can move broken conversations aside, which is not something a page view should do.