Sign up
View stargazersSign up

fix(effect-acp): honor advertised session delete capability#27771

merged2
main
GitHub fork: fix/acp-v1-session-delete
opened11d agobyKKhalil abdalamgeed
2 files changed
View on GitHubTwo-way sync enabledPR last imported 9d ago

Fixes #26927.

ACP clients reject session/delete for every v1-shaped agent, including agents that advertise delete support. This change gates the request on the normalized sessionCapabilities.delete capability and covers v1 and v2 initialize responses, including omitted and null v2 capabilities.

Checks:

  • vp test run --config ../../vite.config.ts --dir . from packages/effect-acp: passed, 68 tests.
  • vp run typecheck from packages/effect-acp: passed.
  • Formatting, lint, and git diff --check: passed.
  • vp run check:changed: format, lint, knip, and tests passed. Typecheck reported 440 mobile React/React Native errors. A baseline comparison on main produced the same exit code and identical diagnostics.

Kiro CLI was unavailable; the in-memory protocol regression covers the reported behavior.

Models and harnesses: GPT-6 Luna and GPT-6.1 Sol via Codex; DeepSeek V4.1 Flash Free and GLM 5.3 Flash Free via OpenCode. GPT-6.1 Terra review was unavailable.

Checks

23 successful · 13 skipped
All checks passed
0a5f9ce

Added 1 commit

  • fix(effect-acp): honor advertised session delete capability9cdbc5a
CcoderabbitaiBot11d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between 9cdbc5aa4faa03227b3a8614a7c798dbf5e4b75f and 0a5f9ce12a70c6eec7cad4090cd01f6eb2e25d7d.

External image Files selected for processing: 2
  • packages/effect-acp/src/client.test.ts
  • packages/effect-acp/src/client.ts

Limit details: You’ve used all 10 included reviews currently available.

Walkthrough

AcpClient records whether an initialized agent advertises session deletion. deleteSession sends the RPC only when that capability is present. Tests cover capability handling for ACP v1 and v2.

Changes: ACP session deletion

Layer / File(s)Summary
Track and apply session deletion capability
packages/effect-acp/src/client.ts, packages/effect-acp/src/client.test.ts		Initialization records deletion support from session capabilities. deleteSession sends the request only when support is present. Tests cover supported and unsupported capability values across ACP v1 and v2.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Possibly related issues

  • pingdotgg/t3code#25382: The change allows runtime.deleteSession to proceed for agents that advertise deletion, but does not add probe cleanup.

Possibly related PRs

  • pingdotgg/t3code#25397: Both changes address ACP session/delete for v1-shaped agents. This change also gates deletion on the advertised capability.

Suggested labels: bugfix

Suggested reviewers: juliusmarminge

Merge Risk: External image Minimal · up to 0a5f9

No concrete merge-blocking risk remains in the reviewed change.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title "fix(effect-acp): honor advertised session delete capability" matches the main change. The summary says deleteSession now sends session/delete only when the negotiated agent advertises the d…
Description checkPassedThe description covers the problem, the change, and verification. It cites the linked issue #26927 and explains that the gate uses the normalized sessionCapabilities.delete value for v1 and v2 respons…
Linked Issues checkPassedIssue [#26927] requires runtime.deleteSession(sessionId) to send session/delete when the agent advertises sessionCapabilities.delete. packages/effect-acp/src/client.ts now uses normalized sess…
Out of Scope Changes checkPassedThe changes are limited to delete-capability tracking and session/delete gating in packages/effect-acp/src/client.ts, with focused regression tests in packages/effect-acp/src/client.test.ts. The…
Full details: Title check

Explanation

The title "fix(effect-acp): honor advertised session delete capability" matches the main change. The summary says deleteSession now sends session/delete only when the negotiated agent advertises the delete capability, for both v1 and v2. The title is short, uses a conventional commit prefix, and names the specific behavior. It does not mention the v1/v2 detail, which is acceptable.

Full details: Description check

Explanation

The description covers the problem, the change, and verification. It cites the linked issue #26927 and explains that the gate uses the normalized sessionCapabilities.delete value for v1 and v2 responses, including omitted and null v2 capabilities. It lists the commands run and their results, and it notes that Kiro CLI could not be tested. It has no separate Problem, Change, or Scope and approval headings. Its approval support is the linked issue, which the objectives describe as requesting this behavior, but it does not include a maintainer approval comment. The model and harness are named at the end, as the template asks. The description is mostly complete, so it passes. The author should consider adding the template headings and a maintainer approval reference.

Full details: Linked Issues check

Explanation

Issue [#26927] requires runtime.deleteSession(sessionId) to send session/delete when the agent advertises sessionCapabilities.delete. packages/effect-acp/src/client.ts now uses normalized session capabilities for both v1 and v2 initialize responses. It sends the request for a non-null delete capability and preserves the local method-not-found failure when the capability is omitted or null. The added parameterized tests cover v1, v2, omitted, and null capability cases.

Full details: Out of Scope Changes check

Explanation

The changes are limited to delete-capability tracking and session/delete gating in packages/effect-acp/src/client.ts, with focused regression tests in packages/effect-acp/src/client.test.ts. These changes directly support issue [#26927]. No unrelated change is demonstrated.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.

Added 1 commit

  • fix(effect-acp): defer session delete capability check0a5f9ce
Mmaria10d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified and fixed at 0a5f9ce12a70c6eec7cad4090cd01f6eb2e25d7d on the existing pr head.

on github/main (ce48e45900), the real codex-acp 2.2.2 provider advertised v1 sessionCapabilities.delete: {}, but t3's initialized runtime rejected deletion with -32601 Method not found: session/delete. the original pr also rejected the cold management path: it evaluated the capability before initialization. the follow-up defers that check with Effect.suspend. the existing regression test now constructs deletion before initialization; its supported v1/v2 rows failed before this fix and pass afterward.

ran the real dev web app with repo playwright and google chrome on linux. created a project pointing at a real git repo and completed real codex gpt-6.1-sol turns through the local acp provider and proxy. through settings, native session deletion returned success and a refreshed list excluded the deleted session. cancelling deletion preserved the session. native session import, a resumed provider turn, and a second turn in the same thread also worked. imported sessions kept their delete control disabled until their t3 thread was removed. a transparent stdio capture confirmed the v1 capability and successful session/delete response {}.

vp run check:changed passed through blacksmith: format, lint, both knip passes, 14 package typechecks, and 33 tests. all current github checks pass. two independent code reviews approved this head.

short runtime transcript

cancel preserves the native session, then confirm deletes it and refresh stays empty

native deletion succeeds in the running dev app

imported native session resumes a real codex turn

Not verified

kiro itself was unavailable. real provider coverage used codex-acp 2.2.2 with the same affected v1 response shape. v2 and absent/null capability cases were exercised by existing protocol tests, not a real v2 provider. native ios/android and electron clients, macos/windows, and remote relay/tunnel connections were not exercised on this linux vm.