fix(server): delete temporary ACP health probe sessions #25395 · pingdotgg/t3code

MmacroscopeappBotapproved these changes8d ago

Approvability

Verdict: Approved at d3983ee

Macroscope's review found this PR approvable — This is a focused ACP health-probe cleanup fix: advertised session/delete support is now used for both v1 and v2 agents, with deletion bounded by a short timeout and ignored on failure. The remaining changes are test coverage and mock-agent support, with no product-default or static-analysis configuration changes.

^{You can add or adjust custom eligibility rules. Learn more.}

Ggithub-actionsBot8d ago

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider	Metric	Main baseline	This PR	Impact	PR ceiling
Codex	Total thread wire	5.0 KiB	5.0 KiB	−7 B (−0.1%)	6.8 KiB	✅
Codex	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Codex	Live turn WebSocket wire	1.2 KiB	1.2 KiB	−7 B (−0.6%)	2.0 KiB	✅
Codex	Live turn WebSocket decoded	20.9 KiB	20.9 KiB	0 B (0.0%)	29.3 KiB	✅
Codex	Live turn messages	2	2	0 (0.0%)	8	✅
Claude	Total thread wire	5.0 KiB	5.0 KiB	+7 B (+0.1%)	6.8 KiB	✅
Claude	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Claude	Live turn WebSocket wire	1.2 KiB	1.2 KiB	+7 B (+0.6%)	2.0 KiB	✅
Claude	Live turn WebSocket decoded	21.2 KiB	21.2 KiB	0 B (0.0%)	29.3 KiB	✅
Claude	Live turn messages	2	2	0 (0.0%)	8	✅

Baseline: 1ecb1a0 · PR result: d3983ee · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

Codex decoded thread snapshot: 108.5 KiB
Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

CcoderabbitaiBot8d ago

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info

External image Run configuration

Source mode: Auto
Repository: pingdotgg/t3code
Branch: fix/issue-25382
File: .coderabbit.config.ts
Commit: d3983ee
Layers: Repository TS config
Review profile: CHILL
Plan: Team
Run ID: 40ddb736-5df0-45b9-8146-7f11043bd89a

External image Commits

Reviewing files that changed from the base of the PR and between 1ecb1a0f9346c7902e98d4362cee0ececd93962c and d3983ee410a844fc92a74adbf1713bc72f820d4a.

External image Files selected for processing: 6

apps/server/scripts/acp-mock-agent.ts
apps/server/src/provider/AcpRegistryProbe.test.ts
packages/effect-acp/src/client.test.ts
packages/effect-acp/src/client.ts
packages/provider-acp-registry/src/server/probe.ts
packages/provider-acp-registry/src/testing.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Walkthrough

The ACP client now sends session/delete for ACP v1. The ACP registry probe registers best-effort cleanup for its test session when deletion is advertised, with a five-second timeout. Tests cover successful and failed deletion outcomes.

Changes: ACP probe cleanup

Layer / File(s)	Summary
ACP v1 session deletion `packages/effect-acp/src/client.ts`, `packages/effect-acp/src/client.test.ts`	`deleteSession` now sends `session/delete` for ACP v1. A test verifies the request and successful response.
Probe session cleanup and validation `packages/provider-acp-registry/src/server/probe.ts`, `packages/provider-acp-registry/src/testing.ts`, `apps/server/scripts/acp-mock-agent.ts`, `apps/server/src/provider/AcpRegistryProbe.test.ts`	The probe registers timed, best-effort deletion when the agent advertises the capability. The mock agent can return a deletion error. Tests check readiness, the advertised capability, and one deletion request for both deletion outcomes.

Priority: External image Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: maria-rcks, t3dotgg

Merge Risk: External image Minimal · up to d3983

The cleanup is ready to merge after normal checks. A rare timeout during startup could still leave an empty probe session.

Pre-merge checks: ^Passed 4

Check name	Status	Explanation
Description check	Passed	The description includes all required sections. It clearly states the problem, explains the cleanup and ACP v1 changes, links the tracked issue and approval context, and provides detailed verification…
Title check	Passed	The title clearly and concisely describes the main change: deleting temporary ACP health probe sessions on the server.
Linked Issues check	Passed	Issue `#25382` requires deletion of probe-created sessions when the agent advertises `sessionCapabilities.delete`, ignored cleanup failures, and support for `session/delete` after ACP v1 negotiation. `p…
Out of Scope Changes check	Passed	All changed files support issue `#25382`. The mock-agent failure switch enables cleanup-error testing. The testing re-export enables probe tests. The client change enables the required v1 deletion RPC. …

Full details: Description check

Explanation

The description includes all required sections. It clearly states the problem, explains the cleanup and ACP v1 changes, links the tracked issue and approval context, and provides detailed verification results.

Full details: Linked Issues check

Explanation

Issue #25382 requires deletion of probe-created sessions when the agent advertises sessionCapabilities.delete, ignored cleanup failures, and support for session/delete after ACP v1 negotiation. probeAcpRegistryConfiguration registers scoped deletion after session start, applies a five-second timeout, and ignores errors. AcpClient.agent.deleteSession now sends the RPC for v1. Tests cover successful and failed probe cleanup and a v1 agent that advertises deletion.

Full details: Out of Scope Changes check

Explanation

All changed files support issue #25382. The mock-agent failure switch enables cleanup-error testing. The testing re-export enables probe tests. The client change enables the required v1 deletion RPC. No unrelated product behavior or client, wire-schema, or platform changes appear in the reviewed diff.

Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

fix(server): delete temporary ACP health probe sessions#25395

Problem

Change

Scope and approval

Verification

Checks

Approvability

Thread transfer impact