feat: conversation context actions — session fork, ephemeral side chat, reply annotations - #347
feat: conversation context actions — session fork, ephemeral side chat, reply annotations#347Awakehsh wants to merge 4 commits into
Conversation
Add complete-turn session forks, including direct cross-agent destinations, and ephemeral tool-free side chats behind a capability gate. Keep reply annotations model-visible while persisting separate display metadata so sent queries retain their annotation attachment after reload and retry. Verification: go test ./...
Reply annotations previously survived only on the message that triggered a run. A follow-up queued while the agent was busy (Desktop enqueues wire text with the full <kocoro_replies> envelope whenever a run is active) reached the model, but its annotations were never decoded into MessageMeta, so the UI chips vanished on reload. Envelope stripping also matched any well-formed tag pair anywhere in a user message, eating user-authored text, and the documented 100/8000/2000 limits only bounded the persistence projection, not what the model received. Forks and side chats dropped the compaction checkpoint, re-feeding (and re-compacting) the full raw archive of compacted sessions. - Decode and persist annotations for every non-injected user message in a run: pre-run mailbox drains merge per-message envelopes (raw bytes) into one head run with merged annotations; mid-run injected turns are handled segment-aware behind the "[New message from user]" prefix (now an exported agent const). - Strip envelopes head-only and only when they parse; a delimited-but- malformed envelope stays verbatim (lossless) instead of evaporating. - Enforce the reply limits at POST /message and POST /queue on the exact envelope bytes, with stable codes conversation_replies_malformed / conversation_replies_too_many / conversation_reply_quote_too_long / conversation_reply_comment_too_long. - Carry a covering CompactionCheckpoint into ForkSessionInto (deep copy) and serve side-chat history via the new Session.HistoryThrough checkpoint+tail view; coverage past the cut is dropped and self-heals. - Validate fork target_agent / side-chat agent existence (400 agent_not_found) so a typo can no longer file sessions into an orphan directory no UI ever lists. - Contract: document the message_index raw-archive basis and the ingress limits in docs/cloud-contract-surface.md; add the http_post.session_fork.response.json wire fixture with a real-producer test and drop the fixture exemption from the README. Verification: go test ./... passes (full suite, exit 0); go test -race passes on internal/session and the new internal/daemon conversation tests. New regression coverage: drained-mailbox and mid-run-injected annotation persistence, head-only + malformed-lossless stripping, ingress limit 400s, SSE side-chat transport, compacted-source fork/side-chat, concurrent forks, unknown-agent rejection, fork response fixture.
…ation
A side chat that needed web_search emitted a literal
<tool_call>web_search(...)</tool_call> as text: the run fed history full of
tool traffic into a loop whose registry was emptied by DisableTools, nothing
told the model it had no tools, and the fabrication detector only knew the
"I called" and <tool_exec> shapes. Product decision: a side chat should be
capability-identical to the primary conversation — only its lifecycle is
ephemeral.
- Drop DisableTools from POST /sessions/{id}/side-chat: side chats run the
normal tool registry and permission engine. Approvals ride the per-request
SSE stream exactly like /message (verified end to end by a live-server
test: approval frame -> POST /approval allow -> tool executes -> done).
The DisableTools runner mechanism itself is unchanged.
- Rewrite the side-chat StickyContext: branched-from-primary framing, tools
are available, the primary conversation must not be modified.
- Gate the SSE handler's QuestionAsker injection on !req.Ephemeral
(shouldInjectQuestionAsker): the side-chat panel has no question UI, so an
asker would block for the auto-resolution window and then report a decline
the user never made. ask_user_question degrades to its clean "can't ask
here" result instead.
- Teach fabricatedToolCallPattern the bare paired
<tool_call>name(args)</tool_call> block. The branch requires paired tags
around a call-shaped body so prose or code discussing the tag stays
unflagged. Backstop, independent of the side-chat change.
- Contract: update the side-chat entry in docs/cloud-contract-surface.md
(tools + approvals + no asker; ephemeral semantics unchanged).
Verification: go test ./... passes (full suite, exit 0). New coverage:
side-chat request carries the registered tool registry, full SSE approval
round-trip on the side-chat route, asker gate table test, fabricated
<tool_call> positives plus unpaired/prose negatives.
![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: a6eca17869
ℹ️ 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".
| abs := i + idx | ||
| atSegmentHead := abs == 0 || strings.HasSuffix(text[:abs], "\n\n") | ||
| if atSegmentHead { | ||
| segVisible, envelope := splitConversationReplyPrompt(text[abs:]) | ||
| if decoded := conversationReplyAnnotations(envelope); envelope != "" && decoded != nil { |
There was a problem hiding this comment.
When an injected follow-up's ordinary body contains a paragraph beginning with \n\n<kocoro_replies>, this blank-line heuristic treats that paragraph as a second message head and removes it from the archived text, even though the ingress contract says tags appearing after the message head are ordinary user content. buildInjectedUserMessage uses the same separator both inside user content and between drained messages, so boundaries must be preserved explicitly rather than inferred from \n\n.
Useful? React with 👍 / 👎.
| for index := range blocks { | ||
| if blocks[index].Type == "text" { | ||
| visible, decoded, stripped := conversationReplyVisibleText(blocks[index].Text) | ||
| if stripped { | ||
| blocks[index].Text = visible | ||
| } | ||
| annotations = decoded | ||
| break |
There was a problem hiding this comment.
When multiple mid-run follow-ups are drained together and any follow-up has a file, buildInjectedUserMessage emits block content whose joined text lacks InjectedUserMessagePrefix. This block branch therefore calls the single-head parser, which strips only the first follow-up's envelope; subsequent envelopes remain in persisted model history and their annotations are lost. The block-based injected path needs the same segment-aware handling as the text-only path.
Useful? React with 👍 / 👎.
| MessageID string `json:"message_id,omitempty"` // stable ID for dedup (e.g. "msg-<uuid>") | ||
| Timestamp *time.Time `json:"timestamp,omitempty"` // when this message was sent/received; nil = legacy (pre-timestamp) | ||
| SystemInjected bool `json:"system_injected,omitempty"` // true for guardrail/nudge messages injected by the agent loop | ||
| ConversationAnnotations []ConversationAnnotation `json:"conversation_annotations,omitempty"` // user-visible reply anchors, excluded from model history |
There was a problem hiding this comment.
conversation_annotations is now serialized in GET /sessions/{id} and explicitly exists for Desktop reconstruction, but neither the session-detail fixture nor its consumer decode test contains this field; the only fixture update in this change covers the fork response. Consequently a producer regression that renames or drops selected_text or comment would pass the wire suite. Add an annotated message to http_get.session.response.json and assert it through the real GET producer path.
AGENTS.md reference: AGENTS.md:L148-L151
Useful? React with 👍 / 👎.
| "github.com/Kocoro-lab/ShanClaw/internal/session" | ||
| ) | ||
|
|
||
| const maxSideChatHistoryMessages = 100 |
There was a problem hiding this comment.
This new max... small-integer cap has no declaration comment explaining the workload behind 100 messages, the client-visible 400 when it binds, or how an operator can override or lift it. Add the required rationale and override guidance at the declaration.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
| maxConversationAnnotations = 100 | ||
| maxConversationAnnotationQuoteRunes = 8_000 | ||
| maxConversationAnnotationCommentRunes = 2_000 |
There was a problem hiding this comment.
These new max... small-integer limits lack the required declaration comments naming the expected reply workload, the rejection or truncation symptom when each limit binds, and the override path. Document those three points for the annotation-count, quote-length, and comment-length caps.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
| mux.HandleFunc("POST /sessions/{id}/fork", s.handleForkSession) | ||
| mux.HandleFunc("POST /sessions/{id}/side-chat", s.handleSideChat) |
There was a problem hiding this comment.
These new Desktop routes and their capability constitute a feature change, but this commit leaves README.md, CLAUDE.md, and AGENTS.md unchanged. Update all three required guides so the full project guide and its condensed mirror document the new surface rather than relying only on the cloud-contract document.
AGENTS.md reference: AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
| | `http_get.sessions.schedule.response.json` | `server.go handleSessions` (`GET /sessions?schedule_id=<id>`) | exact scheduled-task session filter. Each matching row carries persistent `schedule_id`; deleting the schedule configuration does not delete or rewrite the session. | | ||
| | `http_get.session.response.json` | `server.go handleGetSession` (`GET /sessions/{id}`) | lossless default session detail. Top-level `messages` remain the archive; `compaction_checkpoint` additively exposes the durable model-live state and its exclusive raw archive index. No new capability token is required: clients that ignore the additive field retain the existing archive semantics; consumers that opt into live-context metrics read the checkpoint explicitly. | | ||
| | `http_get.session.remote_timeline.response.json` | `server.go handleGetSession` (`GET /sessions/{id}?view=remote_timeline`) | capability-gated mobile projection of the archive. Returns a byte-bounded newest page with aligned `messages` / `message_meta`, absolute `start_index`, opaque `next_cursor`, `has_more`, and explicit `omitted_content_count`; it projects from top-level archived `messages`, not the model-live checkpoint. | | ||
| | `http_post.session_fork.response.json` | `conversation_context.go handleForkSession` (`POST /sessions/{id}/fork`) | fork response for `conversation_context_actions_v1`. `session_id` is the freshly generated fork id (shape-asserted, normalized in the test); `title` snapshots the source session's title. The side-chat route has no fixture of its own: its non-SSE response is a `RunAgentResult` and its SSE stream reuses the already-pinned per-request frames; the boundary, tool-free, and non-persistence contracts are exercised in `internal/daemon/conversation_context_test.go`. | |
There was a problem hiding this comment.
This wire-contract index says the tests exercise a tool-free side-chat contract, but handleSideChat deliberately retains the normal registry and TestSideChatSSEEmitsApprovalRequestAndRunsTool proves an approval-requiring tool executes. Describing the endpoint as tool-free gives maintainers and Desktop consumers the opposite safety semantics; update this entry to say tool-enabled with the normal permission and approval flow.
Useful? React with 👍 / 👎.
1 participant
What
Conversation context actions: fork a session at a complete-turn boundary, run an ephemeral side chat over a session's context, and attach reply annotations (selected passages + comments) to a message.
POST /sessions/{id}/fork— fork at a complete assistant turn, optionally into another agent. Copies the sliced history and, when its coverage boundary fits the cut, the compaction checkpoint — forking a compacted session no longer re-feeds the full raw archive.POST /sessions/{id}/side-chat— ephemeral side conversation over the source session's context (checkpoint + tail). Full tool registry and permission engine; approvals ride the per-request SSE stream; no persistence, no bus events, no question asker (ephemeral runs have no question UI surface, so injection would block and then decline on the user's behalf).<kocoro_replies>head envelope on user messages is decoded intoMessageMeta.ConversationAnnotationsand stripped from persisted visible text; the model receives the original envelope bytes.conversation_context_actions_v1.Hardening
conversation_replies_too_many/conversation_reply_quote_too_long/conversation_reply_comment_too_long/conversation_replies_malformed;agent_not_foundfor unknown fork/side-chat agents. Paths that cannot reject (drain, historical replay) preserve the original text losslessly instead of dropping annotations.<tool_call>…</tool_call>markup (paired tags + call shape; prose about the tag does not match).Contract surfaces
message_index= raw archive index (system-injected messages included); boundary = last included index + 1. Verified against Desktop'sSessionDisplayMapperrawIndex. Documented indocs/cloud-contract-surface.md./statuscapability list updated; newhttp_post.session_fork.response.jsonwith a real-producer test.scripts/copy-shan-to-helper.sh), then the Desktop PR.How to test
go test ./...— all green, including-raceoninternal/sessionand the new daemon tests (SSE side chat with approval round-trip, concurrent fork, compacted-source fork, drain persistence, envelope validation, head-only stripping).web_searchcalls and returned a cited answer; the ephemeral session left no file on disk and no/sessionsentry; fork with an unknowntarget_agentreturned400 {"code":"agent_not_found"}.