Skip to content

Feature: ACP (Agent Client Protocol) Notification Target #976

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tbrandenburg merged 7 commits into from
Feb 2, 2026
Merged

Feature: ACP (Agent Client Protocol) Notification Target #976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a42a7a7
feat: Add ACP notification target (MVP Phase 1)
Feb 2, 2026
18cce4e
fix: ACP handler process cleanup and E2E test
Feb 2, 2026
17d6096
docs: add final verification report with real OpenCode testing
Feb 2, 2026
f989e79
docs: add ACP target documentation with session limitation
Feb 2, 2026
9819c27
test: increase ACP handler coverage from 14% to 96%+
Feb 2, 2026
da2c654
Fix: Enable ACP session persistence across CLI invocations (#963)
Feb 2, 2026
6f20ea5
Fix: Add explicit return type to cleanup function
Feb 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,389 changes: 1,389 additions & 0 deletions .claude/PRPs/plans/completed/opencode-acp-notification-target.plan.md
View file
Open in desktop

Large diffs are not rendered by default.

370 changes: 370 additions & 0 deletions .claude/PRPs/reports/acp-session-persistence-verification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,370 @@
# Implementation Verification Report

**Plan**: `.claude/PRPs/plans/completed/opencode-acp-notification-target.plan.md`
**Verification Date**: 2026-02-02T10:59:45Z
**Status**: ⚠️ CRITICAL ISSUES FOUND

---

## Executive Summary

**Tasks Completed**: 9/10
**Validations Passing**: 4/6
**Critical Issues**: 1
**Minor Issues**: 2

**BLOCKER**: Session persistence is NOT implemented despite being a core requirement in the plan and acceptance criteria.

---

## Critical Findings (BLOCKERS)

### ❌ Finding 1: Session Persistence Not Implemented

**Task**: Task 3 - ACP Handler Implementation
**Plan Requirement**: "Session IDs persisted in config for reuse" (Acceptance Criteria line 1228)
**Plan Requirement**: "Update config with session ID for persistence" (Task 3, line 485)
**Plan Requirement**: "sessionId stored in .work/contexts.json (survive CLI restarts)" (Notes, line 1301)

**Issue**: The `ACPTargetConfig` interface uses `readonly` properties, preventing the handler from updating the sessionId after initialization. The implementation explicitly acknowledges this limitation:

```typescript
// src/core/target-handlers/acp-handler.ts:53-55
// NOTE: sessionId is NOT persisted to config (readonly).
// Process reuse means same session within a command, but not across commands.
// TODO: Update context manager to allow handlers to persist state
```

**Evidence**:
```bash
# ACPTargetConfig is readonly
$ grep -A5 "interface ACPTargetConfig" src/types/notification.ts
export interface ACPTargetConfig {
readonly type: 'acp';
readonly cmd: string;
readonly cwd?: string;
readonly timeout?: number;
readonly sessionId?: string; # ← READONLY - cannot be updated
}
```

**E2E Test Also Skips Verification**:
```typescript
// tests/e2e/acp-integration.test.ts:116-128
// Verify session was persisted (Note: currently NOT persisted due to readonly config)
// This is a known limitation - sessionId won't persist across commands
// TODO: Once context manager allows handler state persistence, re-enable this check
// ... commented out verification code ...
```

**Impact**:
- Sessions are created fresh on every CLI invocation (5-7s initialization penalty each time)
- Violates plan's MVP Phase 1 requirement: "Session persistence (hardcoded `persist` strategy)"
- Acceptance criteria explicitly states: "Session IDs persisted in config for reuse"
- Phase 2 cannot proceed without this foundation

**Required Action**:
- [ ] Modify `TargetConfig` types to allow mutable `sessionId` field OR
- [ ] Implement handler state persistence mechanism in `WorkEngine` OR
- [ ] Add `updateNotificationTarget()` method to allow handler to save state OR
- [ ] Update plan to clarify session persistence is Phase 2 (scope change)

**Root Cause**: TypeScript `readonly` modifier on config interfaces conflicts with runtime state mutation needs. The plan assumed handlers could update config, but the type system prevents it.

---

## Minor Findings (IMPROVEMENTS)

### ⚠️ Finding 2: Linting Warning in engine.ts

**Task**: Task 5 - Register Handler in Engine
**Issue**: Missing return type annotation on cleanup function

**Evidence**:
```bash
$ npm run lint
/home/tom/workspace/ai/made/workspace/work/src/core/engine.ts
76:24 warning Missing return type on function @typescript-eslint/explicit-function-return-type
```

**Required Action**:
- [ ] Add explicit return type to function on line 76 in `src/core/engine.ts`

**Priority**: LOW - Does not affect functionality, only code style

---

### ⚠️ Finding 3: Coverage Target Unverified

**Task**: Task 10 - Full Validation
**Plan Requirement**: "Coverage >= 40%" (Task 10, line 1085)

**Issue**: Test suite takes >120s to run. Could not verify final coverage percentage within timeout.

**Partial Evidence**:
- Unit tests pass: 21/22 passing (1 skipped) in `acp-handler.test.ts`
- E2E tests pass: 4/4 passing in `acp-integration.test.ts`
- All other tests appear to pass based on partial output

**Required Action**:
- [ ] Run `npm run test:coverage` to completion
- [ ] Verify coverage >= 40% for MVP acceptance
- [ ] Check coverage for `acp-handler.ts` specifically (plan suggests >= 80% for handler)

**Priority**: MEDIUM - Required for acceptance criteria but likely met

---

## Verification Status by Category

### Tasks Implementation

✅ **9/10 tasks fully implemented**:
1. ✅ Task 1: Types updated (`src/types/notification.ts`)
2. ✅ Task 2: Error classes added (`src/types/errors.ts`)
3. ⚠️ Task 3: Handler created BUT session persistence broken
4. ✅ Task 4: Exports added (`src/core/target-handlers/index.ts`)
5. ✅ Task 5: Handler registered in engine (with 1 lint warning)
6. ✅ Task 6: CLI flags added (`--cmd`, `--cwd`)
7. ✅ Task 7: Unit tests created (21/22 passing)
8. ✅ Task 8: E2E tests created (4/4 passing)
9. ✅ Task 9: Documentation updated (MVP Phase 1 marked complete)
10. ❌ Task 10: Full validation incomplete (coverage not verified)

### Validation Gates

| Level | Command | Status | Details |
|-------|---------|--------|---------|
| 1 | `npm run lint && npm run type-check` | ⚠️ | Type-check ✅, Lint has 1 warning |
| 2 | `npm run build && ./bin/run.js notify target --help` | ✅ | Build succeeds, help displays ACP |
| 3 | `npm run test:unit -- acp-handler.test.ts --coverage` | ✅ | 21/22 tests pass |
| 4 | `npm run test:coverage && npm run build` | ⚠️ | Tests pass but coverage % unverified |
| 5 | `npm run test -- tests/e2e/acp-integration.test.ts` | ✅ | 4/4 E2E tests pass |
| 6 | Manual validation | ❌ | Not executed (requires OpenCode auth) |

### Quality Metrics

- **Type Safety**: ✅ No TypeScript errors
- **Linting**: ⚠️ 1 warning (missing return type)
- **Unit Tests**: ✅ 21/22 passing (95%)
- **E2E Tests**: ✅ 4/4 passing (100%)
- **Coverage**: ⚠️ Not verified (target: 40%)
- **Build**: ✅ Succeeds without errors
- **File Changes**: ✅ All expected files created/updated

---

## Acceptance Criteria Status

From plan lines 1219-1238:

- [x] `TargetType` includes 'acp' option
- [x] `ACPTargetConfig` interface defined (generic)
- [x] Generic ACP error classes created
- [x] `ACPTargetHandler` implements `TargetHandler` interface
- [x] Handler spawns ANY ACP client subprocess via `--cmd` parameter
- [x] JSON-RPC 2.0 messages sent/received over stdin/stdout
- [x] Sessions initialized with `initialize` + `session/new`
- [❌] **Session IDs persisted in config for reuse** ← **BLOCKER**
- [x] CLI accepts `--type acp --cmd <any-acp-client>` flags
- [⚠️] Level 1-4 validation commands pass (mostly, with warnings)
- [⚠️] Unit tests cover >= 80% of handler code (unverified)
- [x] E2E tests verify full workflow using OpenCode
- [x] Code mirrors existing patterns
- [x] No regressions in existing tests
- [x] Process cleanup on exit
- [x] MVP documentation updated
- [x] Implementation is generic

**Score**: 15/17 verified (88%), 1 critical blocker, 2 unverified

---

## Detailed Verification Evidence

### File-by-File Check

#### src/types/notification.ts
```typescript
✅ Line 5: export type TargetType = 'bash' | 'telegram' | 'email' | 'acp';
✅ Lines 38-44: ACPTargetConfig interface with all required fields
❌ Lines 39-43: All fields marked `readonly` - prevents persistence
```

#### src/types/errors.ts
```typescript
✅ Lines 84-94: ACPError class
✅ Lines 96-102: ACPTimeoutError class
✅ Lines 104-114: ACPInitError class
✅ Lines 116-122: ACPSessionError class
```

#### src/core/target-handlers/acp-handler.ts
```typescript
✅ Lines 25-34: ACPTargetHandler class structure
✅ Lines 36-74: send() method with proper error handling
✅ Lines 76-105: ensureProcess() with subprocess spawning
✅ Lines 107-126: Message buffering and JSON-RPC parsing
✅ Lines 164-195: Session initialization (initialize + session/new)
✅ Lines 218-251: JSON-RPC request/response handling
❌ Lines 53-55: Explicit TODO about session persistence not working
✅ Lines 59,67: Process cleanup on exit
```

#### src/core/engine.ts
```typescript
✅ Line 31: import { ACPTargetHandler } from './target-handlers/acp-handler.js';
✅ Line 39: private acpHandler: ACPTargetHandler | null = null;
✅ Line 68-69: Handler instantiated and registered
⚠️ Line 76: Missing return type annotation (lint warning)
```

#### src/cli/commands/notify/target/add.ts
```typescript
✅ Line 41: 'acp' added to type options
✅ Lines 64-68: --cmd flag defined
✅ Lines 69-72: --cwd flag defined
✅ Lines 93-95: Validation for --cmd requirement
✅ Lines 138-147: Config building for ACP type
```

#### tests/unit/core/target-handlers/acp-handler.test.ts
```typescript
✅ 22 test cases covering:
- formatWorkItems (4 tests)
- send() method (3 tests)
- ensureProcess() (5 tests)
- sendRequest() (3 tests)
- setupMessageHandler() (2 tests)
- cleanup() (2 tests)
- error handling (3 tests)
✅ 21/22 passing (1 skipped test)
```

#### tests/e2e/acp-integration.test.ts
```typescript
✅ 4 test cases covering:
- Add target
- Send notification (with real OpenCode)
- List targets
- Remove target
✅ All 4 tests passing
❌ Lines 116-128: Session persistence check commented out
```

#### dev/poc-opencode-server/WORK-CLI-INTEGRATION.md
```bash
$ grep "MVP (Phase 1)" -A5 dev/poc-opencode-server/WORK-CLI-INTEGRATION.md
### MVP (Phase 1): ✅ COMPLETE (2026-02-02)
✅ Marked complete on line 669
```

---

## Action Items (Priority Order)

### 🔥 Critical (Must Fix)

1. **Implement Session Persistence** (BLOCKER)
- [ ] Choose approach:
- Option A: Remove `readonly` from `sessionId` in `ACPTargetConfig`
- Option B: Add `updateNotificationTarget()` to `WorkEngine`
- Option C: Implement handler state persistence layer
- [ ] Update handler to save `sessionId` after creation
- [ ] Re-enable E2E test verification (lines 116-128)
- [ ] Verify session persists across CLI invocations
- **Estimated Time**: 2-4 hours
- **Complexity**: MEDIUM (requires type system + engine changes)

### ⚠️ Important (Should Fix)

2. **Verify Coverage Target**
- [ ] Run `npm run test:coverage` to completion
- [ ] Confirm overall coverage >= 40%
- [ ] Confirm handler coverage >= 80%
- **Estimated Time**: 10 minutes
- **Complexity**: LOW

3. **Fix Linting Warning**
- [ ] Add return type to function on line 76 in `src/core/engine.ts`
- [ ] Run `npm run lint` to verify
- **Estimated Time**: 2 minutes
- **Complexity**: LOW

### 💡 Minor (Nice to Fix)

4. **Manual Functional Testing**
- [ ] Complete Level 6 validation (manual CLI testing)
- [ ] Verify with real OpenCode authentication
- **Estimated Time**: 15 minutes
- **Complexity**: LOW

---

## Plan vs Reality Gap Analysis

### What Plan Expected
1. **Session Persistence**: "Update config with session ID for persistence" (line 485)
2. **Config Mutation**: "config.sessionId = sessionId;" (line 485)
3. **Persistent State**: "sessionId stored in .work/contexts.json" (line 1301)

### What Was Delivered
1. **No Session Persistence**: Config is `readonly`, cannot be mutated
2. **In-Memory Only**: Sessions exist only within single CLI invocation
3. **Explicit TODO**: Code acknowledges this limitation

### Root Cause
The plan did not account for TypeScript's `readonly` modifier on config interfaces. All `TargetConfig` types use `readonly` for immutability, but ACP handler needs to update state after initialization.

### Recommended Fix
**Option A (Simplest)**: Remove `readonly` from `sessionId` field only:
```typescript
export interface ACPTargetConfig {
readonly type: 'acp';
readonly cmd: string;
readonly cwd?: string;
readonly timeout?: number;
sessionId?: string; // ← Remove readonly here
}
```

Then update handler to mutate and save:
```typescript
config.sessionId = sessionId;
// Trigger context save (requires WorkEngine support)
```

---

## Next Steps

1. **Address Critical Issue**: Fix session persistence (see Option A above)
2. **Re-run Verification**: Execute this check again after fix
3. **Verify Coverage**: Run full test suite with coverage reporting
4. **Fix Minor Issues**: Lint warning + manual testing
5. **Final Acceptance**: Confirm all 17 acceptance criteria met

**Estimated Total Fix Time**: 3-5 hours
**Complexity**: MEDIUM (type system changes + engine updates)

---

## Conclusion

The implementation is **85% complete** with high code quality and good test coverage. The critical blocker is **session persistence**, which was explicitly required by the plan but not implemented due to `readonly` type constraints.

**Recommendation**: Fix session persistence before marking Phase 1 complete. This is a foundational requirement for Phase 2 enhancements (model selection, session strategies, etc.).

Without session persistence:
- ❌ Users pay 5-7s initialization cost on every command
- ❌ No conversation context across CLI invocations
- ❌ Violates plan's MVP requirements
- ❌ Cannot proceed to Phase 2 features

With the fix:
- ✅ Sessions persist across commands
- ✅ Fast subsequent invocations
- ✅ True conversational AI integration
- ✅ Ready for Phase 2

**Status**: Implementation is functionally complete but architecturally incomplete. One critical fix required for acceptance.
Loading