|
| 1 | +# oho add --no-reply Bug Fix Implementation Plan |
| 2 | + |
| 3 | +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. |
| 4 | +
|
| 5 | +**Goal:** Fix the `oho add --no-reply` command so that it properly verifies message delivery and provides clear feedback to users. |
| 6 | + |
| 7 | +**Architecture:** The issue is that `--no-reply` mode sends to `/session/{id}/message` with `noReply=true`, but treats empty server response as success without verification. The fix adds explicit response validation and clear messaging about async message status. |
| 8 | + |
| 9 | +**Tech Stack:** Go, Cobra CLI, HTTP Client |
| 10 | + |
| 11 | +--- |
| 12 | + |
| 13 | +## Root Cause Summary |
| 14 | + |
| 15 | +When `oho add "message" --no-reply` is executed: |
| 16 | +1. Session is created successfully |
| 17 | +2. Message is sent to `/session/{id}/message` with `noReply=true` |
| 18 | +3. Server returns empty body `[]byte{}` (async processing) |
| 19 | +4. Client returns `(messageID="", err=nil)` - **false success** |
| 20 | +5. User sees "Message sent successfully" but message may not be queued |
| 21 | + |
| 22 | +**The Fix:** Add verification for no-reply mode by using a different approach: |
| 23 | +- Option A: Use `/session/{id}/prompt_async` endpoint (dedicated async endpoint) |
| 24 | +- Option B: Verify message was stored by checking session messages list |
| 25 | +- Option C: Return the session ID so user can verify manually |
| 26 | + |
| 27 | +**Recommended Fix:** Option A - Use proper async endpoint |
| 28 | + |
| 29 | +--- |
| 30 | + |
| 31 | +## Atomic Commit Strategy |
| 32 | + |
| 33 | +| Commit | Description | Files | |
| 34 | +|--------|-------------|-------| |
| 35 | +| 1 | Add verification test for no-reply mode | `oho/cmd/add/add_test.go` | |
| 36 | +| 2 | Fix sendMessage to use prompt_async for no-reply mode | `oho/cmd/add/add.go` | |
| 37 | +| 3 | Update error message for no-reply mode to be clearer | `oho/cmd/add/add.go` | |
| 38 | + |
| 39 | +--- |
| 40 | + |
| 41 | +## Task 1: Write Failing Test for No-Reply Mode Verification |
| 42 | + |
| 43 | +**Files:** |
| 44 | +- Modify: `oho/cmd/add/add_test.go` |
| 45 | + |
| 46 | +**Step 1: Add test case for no-reply verification** |
| 47 | + |
| 48 | +Add a new test case that verifies when `noReply=true`: |
| 49 | +- The mock should be called with the correct endpoint |
| 50 | +- The response handling should be correct |
| 51 | + |
| 52 | +```go |
| 53 | +// Add to TestSendMessage test cases: |
| 54 | +{ |
| 55 | + name: "no-reply mode uses correct endpoint", |
| 56 | + sessionID: "ses_test123", |
| 57 | + message: "Hello", |
| 58 | + noReply: true, |
| 59 | + mockResp: []byte{}, // Empty response for no-reply |
| 60 | + mockErr: nil, |
| 61 | + wantErr: false, |
| 62 | + wantMsgID: "", // No message ID returned |
| 63 | + verifyPath: "/session/ses_test123/prompt_async", // Should use async endpoint |
| 64 | +}, |
| 65 | +``` |
| 66 | + |
| 67 | +Wait - the issue is the endpoint should be `/prompt_async` not `/message`. Let me verify current behavior first. |
| 68 | + |
| 69 | +**Step 2: Run existing tests to understand current behavior** |
| 70 | + |
| 71 | +Run: `cd /mnt/d/fe/opencode_cli && go test -v ./oho/cmd/add/... -run TestSendMessage` |
| 72 | +Expected: All tests pass (baseline) |
| 73 | + |
| 74 | +--- |
| 75 | + |
| 76 | +## Task 2: Investigate Correct API Behavior |
| 77 | + |
| 78 | +**Files:** |
| 79 | +- None (investigation) |
| 80 | + |
| 81 | +**Step 1: Check OpenCode API documentation** |
| 82 | + |
| 83 | +The API mapping shows: |
| 84 | +- `oho message prompt-async` → `/session/:id/prompt_async` (POST) |
| 85 | + |
| 86 | +But `oho add --no-reply` uses `/session/:id/message` with `noReply=true`. |
| 87 | + |
| 88 | +**Step 2: Determine correct approach** |
| 89 | + |
| 90 | +The issue is: |
| 91 | +- `/session/{id}/message` with `noReply=true` should return a message ID (even if empty body initially) |
| 92 | +- But server returns empty body, causing ambiguity |
| 93 | + |
| 94 | +**Recommended fix:** When `noReply=true`, use `/session/{id}/prompt_async` endpoint instead, which is designed for async messaging. |
| 95 | + |
| 96 | +--- |
| 97 | + |
| 98 | +## Task 3: Implement the Fix |
| 99 | + |
| 100 | +**Files:** |
| 101 | +- Modify: `oho/cmd/add/add.go:197-261` (sendMessage function) |
| 102 | + |
| 103 | +**Step 1: Update sendMessage to use prompt_async for no-reply mode** |
| 104 | + |
| 105 | +```go |
| 106 | +// sendMessage sends a message to the session and returns the message ID |
| 107 | +func sendMessage(c client.ClientInterface, ctx context.Context, sessionID, message, agent, model string, noReply bool, system string, tools, files []string) (string, error) { |
| 108 | + // Build message parts |
| 109 | + var parts []types.Part |
| 110 | + |
| 111 | + // Add text part |
| 112 | + text := message |
| 113 | + parts = append(parts, types.Part{ |
| 114 | + Type: "text", |
| 115 | + Text: &text, |
| 116 | + }) |
| 117 | + |
| 118 | + // Add file parts |
| 119 | + for _, filePath := range files { |
| 120 | + // ... file handling code (lines 209-233) |
| 121 | + } |
| 122 | + |
| 123 | + // Build message request |
| 124 | + msgReq := types.MessageRequest{ |
| 125 | + Model: convertModel(model), |
| 126 | + Agent: agent, |
| 127 | + NoReply: noReply, |
| 128 | + System: system, |
| 129 | + Tools: tools, |
| 130 | + Parts: parts, |
| 131 | + } |
| 132 | + |
| 133 | + // Determine endpoint based on noReply flag |
| 134 | + endpoint := fmt.Sprintf("/session/%s/message", sessionID) |
| 135 | + if noReply { |
| 136 | + endpoint = fmt.Sprintf("/session/%s/prompt_async", sessionID) |
| 137 | + // For async endpoint, always set noReply to false (server handles async internally) |
| 138 | + msgReq.NoReply = false |
| 139 | + } |
| 140 | + |
| 141 | + resp, err := c.Post(ctx, endpoint, msgReq) |
| 142 | + if err != nil { |
| 143 | + return "", fmt.Errorf("API request failed: %w", err) |
| 144 | + } |
| 145 | + |
| 146 | + // Handle empty response (no-reply mode) |
| 147 | + if len(resp) == 0 { |
| 148 | + // For no-reply mode, we can't verify message ID, but we can confirm it was accepted |
| 149 | + if noReply { |
| 150 | + // Return session ID as reference since message ID is not available |
| 151 | + return sessionID, nil |
| 152 | + } |
| 153 | + return "", nil |
| 154 | + } |
| 155 | + |
| 156 | + var result types.MessageWithParts |
| 157 | + if err := json.Unmarshal(resp, &result); err != nil { |
| 158 | + return "", fmt.Errorf("failed to parse response: %w", err) |
| 159 | + } |
| 160 | + |
| 161 | + return result.Info.ID, nil |
| 162 | +} |
| 163 | +``` |
| 164 | + |
| 165 | +**Step 2: Update output message in runAdd to clarify async behavior** |
| 166 | + |
| 167 | +In `runAdd` function (around line 136-142): |
| 168 | + |
| 169 | +```go |
| 170 | +} else { |
| 171 | + fmt.Printf("Session created: %s\n", sessionID) |
| 172 | + if messageID != "" { |
| 173 | + fmt.Printf("Message sent: %s\n", messageID) |
| 174 | + } else if addNoReply { |
| 175 | + // For no-reply mode, message ID won't be returned, but session was created |
| 176 | + fmt.Printf("Message sent (async mode - session: %s)\n", sessionID) |
| 177 | + } else { |
| 178 | + fmt.Println("Message sent successfully") |
| 179 | + } |
| 180 | +} |
| 181 | +``` |
| 182 | + |
| 183 | +**Step 3: Run tests to verify fix** |
| 184 | + |
| 185 | +Run: `cd /mnt/d/fe/opencode_cli && go test -v ./oho/cmd/add/... -run TestSendMessage` |
| 186 | +Expected: Tests should pass (including new no-reply test) |
| 187 | + |
| 188 | +--- |
| 189 | + |
| 190 | +## Task 4: Add Integration Test for No-Reply Mode |
| 191 | + |
| 192 | +**Files:** |
| 193 | +- Modify: `oho/cmd/add/add_test.go` |
| 194 | + |
| 195 | +**Step 1: Add integration test case for no-reply with session verification** |
| 196 | + |
| 197 | +```go |
| 198 | +func TestNoReplyModeVerification(t *testing.T) { |
| 199 | + // Test that no-reply mode properly creates session and accepts message |
| 200 | + mock := &client.MockClient{ |
| 201 | + PostWithQueryFunc: func(ctx context.Context, path string, queryParams map[string]string, body interface{}) ([]byte, error) { |
| 202 | + return testutil.MockSessionResponse(), nil |
| 203 | + }, |
| 204 | + PostFunc: func(ctx context.Context, path string, body interface{}) ([]byte, error) { |
| 205 | + // Verify the endpoint used |
| 206 | + if path != "/session/session1/prompt_async" { |
| 207 | + t.Errorf("Expected /session/session1/prompt_async, got %s", path) |
| 208 | + } |
| 209 | + return []byte{}, nil // Empty response for async |
| 210 | + }, |
| 211 | + } |
| 212 | + |
| 213 | + ctx := context.Background() |
| 214 | + |
| 215 | + // Create session |
| 216 | + sessionID, err := createSession(mock, ctx, "Test", "", "/test") |
| 217 | + if err != nil { |
| 218 | + t.Fatalf("Failed to create session: %v", err) |
| 219 | + } |
| 220 | + |
| 221 | + // Send message with noReply=true |
| 222 | + msgID, err := sendMessage(mock, ctx, sessionID, "Test message", "", "", true, "", nil, nil) |
| 223 | + if err != nil { |
| 224 | + t.Errorf("Unexpected error in no-reply mode: %v", err) |
| 225 | + } |
| 226 | + |
| 227 | + // For no-reply mode, msgID should be sessionID (as fallback reference) |
| 228 | + t.Logf("No-reply mode returned session ID as reference: %s", msgID) |
| 229 | +} |
| 230 | +``` |
| 231 | + |
| 232 | +--- |
| 233 | + |
| 234 | +## Task 5: Update Test Mock Client |
| 235 | + |
| 236 | +**Files:** |
| 237 | +- Modify: `oho/internal/client/client_mock.go` |
| 238 | + |
| 239 | +**Step 1: Verify MockClient supports PostFunc properly** |
| 240 | + |
| 241 | +Check that the mock can track which endpoint was called. |
| 242 | + |
| 243 | +```go |
| 244 | +// client_mock.go should have: |
| 245 | +type MockClient struct { |
| 246 | + PostWithQueryFunc func(ctx context.Context, path string, queryParams map[string]string, body interface{}) ([]byte, error) |
| 247 | + PostFunc func(ctx context.Context, path string, body interface{}) ([]byte, error) |
| 248 | + // Add for tracking: |
| 249 | + LastPostPath string // Track last POST path for verification |
| 250 | +} |
| 251 | +``` |
| 252 | + |
| 253 | +--- |
| 254 | + |
| 255 | +## Task 6: Run Full Test Suite |
| 256 | + |
| 257 | +**Files:** |
| 258 | +- None |
| 259 | + |
| 260 | +**Step 1: Run all add command tests** |
| 261 | + |
| 262 | +Run: `cd /mnt/d/fe/opencode_cli && go test -v ./oho/cmd/add/...` |
| 263 | +Expected: All tests pass |
| 264 | + |
| 265 | +**Step 2: Run all oho tests** |
| 266 | + |
| 267 | +Run: `cd /mnt/d/fe/opencode_cli && go test ./oho/...` |
| 268 | +Expected: All tests pass |
| 269 | + |
| 270 | +**Step 3: Verify build** |
| 271 | + |
| 272 | +Run: `cd /mnt/d/fe/opencode_cli/oho && go build -o oho_test ./cmd/oho/` |
| 273 | +Expected: Build succeeds |
| 274 | + |
| 275 | +--- |
| 276 | + |
| 277 | +## Task 7: Commit Changes |
| 278 | + |
| 279 | +**Files:** |
| 280 | +- `oho/cmd/add/add.go` |
| 281 | +- `oho/cmd/add/add_test.go` |
| 282 | + |
| 283 | +**Step 1: Stage and commit** |
| 284 | + |
| 285 | +```bash |
| 286 | +git add oho/cmd/add/add.go oho/cmd/add/add_test.go |
| 287 | +git commit -m "fix: use /prompt_async endpoint for --no-reply mode |
| 288 | +
|
| 289 | +Previously, oho add --no-reply sent to /session/{id}/message with |
| 290 | +noReply=true, which returned an empty response treated as success |
| 291 | +without verification. |
| 292 | +
|
| 293 | +Now, --no-reply mode uses the dedicated /session/{id}/prompt_async |
| 294 | +endpoint which is designed for async message handling. |
| 295 | +
|
| 296 | +Fixes: Session created but message not properly queued issue. |
| 297 | +" |
| 298 | +``` |
| 299 | + |
| 300 | +--- |
| 301 | + |
| 302 | +## Verification Checklist |
| 303 | + |
| 304 | +- [ ] `go test ./oho/cmd/add/...` passes |
| 305 | +- [ ] `go test ./oho/...` passes |
| 306 | +- [ ] `go build ./cmd/oho/...` succeeds |
| 307 | +- [ ] Manual test: `oho add "test" --no-reply` works correctly |
| 308 | +- [ ] Output clearly indicates async message was accepted |
0 commit comments