Repository navigation
Expose declared plans through the sessions MCP recall tools - #1135
Conversation
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The plans server prompts on every call and is shut out of unattended review flows, so recall could not reach a plan a dead session left behind. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
BuildRepoSessionsUrl and BuildRepoPlansUrl repeated the same 11-line repo/cwd-hash block verbatim. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
An agent recapping a dead session had no way to learn a task ledger existed. The lookup is bounded and fails open, since the stdio loop is serial. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Declaring a plan's document from another worktree forks the plan, and a task snapshot sent from a partial view destroys what the caller cannot see; the resume procedure is ordered around both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…1128) The plans route answers for a session's whole continuation chain, so every description that names the session says so. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR Summary by QodoExpose declared plans through sessions MCP recall
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 286d6b9468
ℹ️ 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".
| internal static string BuildSessionPlansUrl(string baseUrl, string sessionId) => | ||
| $"{baseUrl}/api/sessions/{Uri.EscapeDataString(sessionId)}/plans"; |
There was a problem hiding this comment.
Reject dot-segment session IDs
When get_declared_plans receives session_id: "." or "..", Uri.EscapeDataString leaves the value unchanged and the HTTP URI normalizes the dot segment, so the request targets /api/sessions/plans or /api/plans instead of the session-plans endpoint. Validate these values (or canonicalize session IDs as SessionPlansClient does) before constructing the URL, just as this change already does for plan_id.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 917b600 — BuildSessionPlansUrl rejects . and .. with the same ArgumentException shape as the plan_id guard, and since the summary handler now builds that URL before any request, get_session_summary is covered by the same check. Unit test BuildDeclaredPlansUrl_rejects_a_dot_segment_session_id.
| using var recap = await client.GetAsync(recapUrl); | ||
| var body = await recap.Content.ReadAsStringAsync(); | ||
| var plans = await plansTask; |
There was a problem hiding this comment.
Cancel the plans request when recap retrieval fails
If the recap request throws while the parallel plans request is still stalled, control enters the catch without awaiting or cancelling plansTask; disposing plansCts does not signal cancellation and also removes its scheduled timeout. The lookup can therefore outlive the tool call until the shared HttpClient timeout, and repeated recap failures can accumulate background requests. Cancel and observe the plans task on this exception path so the advertised 10-second bound applies on every exit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 917b600 — the recap read now runs inside a try/finally: every exit, including a thrown recap request, cancels plansCts and awaits the plans task (which never throws), so the lookup ends with the tool call instead of outliving it. Pinned by Get_session_summary_returns_a_failed_recap_without_waiting_for_a_stalled_plans_lookup: against the previous handler it failed at 11.2 s; it now returns well under 5 s and the next stdio request is not held.
Code Review by Qodo
1.
|
A failed or unauthorized recap must not wait out the plans lookup's 10 s bound, and a dot segment in session_id must not escape the route. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Closes #1128 — AI-3036
What & why
A session that dies mid-plan leaves a task ledger recall cannot reach: a summary carries only the captured plan text,
kcap-plansprompts on every call and is shut out of unattended review flows, and nothing finds a plan without a session id in hand. The read-onlykcap mcp sessionsserver now relays the server's plan reads —list_repo_planslists a repository's open (or all) declared plans with each one's next task and the liveness of its attached sessions,get_declared_plansreads one plan by id or every plan a session touched — andget_session_summarypoints at the session's plans throughdeclared_plans, fetched beside/recapunder a bound and dropped on any failure. Therecapandplansskills teach an agent to judge done-ness fromfinishedrather thanis_complete(which only says nothing was withheld from the view) and to resume a plan in an order that never snapshots from a partial view, never declares the document from another checkout, and adopts last. The server half is kurrent-io/kcap-server#2009; against a server without the route,list_repo_plansanswers with a plain "not yet" message, and wherefinishedis absent the CLI derives it astotal_known && completed == total && is_complete.Where to look
HandleSessionSummaryAsync: the plans call starts before the recap read and is awaited after it, so a stalled/planscan hold a summary for at most the 10 s bound but never fails it. The resume procedure inkcap/skills/plans/SKILL.mdis ordered around data-loss traps in the ledger's write path; keep its order if you edit it.Verification
Before the merge of main: unit 4670/4670 (two session-start hook tests failed under a load average above 50 and passed alone), integration 301/301 at
--maximum-parallel-tests 2(the stdio tests, pre-existing ones included, flake at full parallelism on a loaded host),dotnet publish -c Releasewith zero IL2026/IL3050. On the merged tip:McpSessionsServerTests75/75 andKcapMcpRegistryReviewFlowTests16/16 (the registry-set mutation check failsSessions_server_advertises_exactly_its_unattended_safe_tool_setwhen either tool is dropped), integrationMcpSessionsServerTests18/18,scripts/check-linear-ids.shclean. The 404, fail-open and recap-fails paths are pinned by WireMock integration tests; the with-route path has not been driven against a live server, since none carries the route until the server PR ships.🤖 Generated with Claude Code