|
| 1 | +# Fix: `seed-cli comment create --reply` fails after comment edit |
| 2 | + |
| 3 | +## Bug summary |
| 4 | + |
| 5 | +`seed-cli comment create <target> --reply <commentId>` fails with `"Non-base58btc character"` when the reply parent (or any ancestor in the chain) was previously edited via `seed-cli comment edit`. |
| 6 | + |
| 7 | +## Reproduction steps |
| 8 | + |
| 9 | +1. Post comment A on a document |
| 10 | +2. Post comment B with `--reply A` -- works, threaded correctly |
| 11 | +3. Edit comment B's body via `seed-cli comment edit B --body "new text"` -- creates new CID version |
| 12 | +4. Post comment C with `--reply B` -- **fails** with `Non-base58btc character` |
| 13 | +5. Posting C without `--reply` works but loses threading |
| 14 | + |
| 15 | +## Root cause analysis |
| 16 | + |
| 17 | +The bug is in the CLI's `comment create --reply` handler and in the `@seed-hypermedia/client` library's `createSignedComment` function. There are **two separate problems** in the data flow: |
| 18 | + |
| 19 | +### Problem 1: CLI passes RecordID where CID is expected (comment.ts lines 126-135) |
| 20 | + |
| 21 | +File: `frontend/apps/cli/src/commands/comment.ts` |
| 22 | + |
| 23 | +```typescript |
| 24 | +if (options.reply) { |
| 25 | + const parentComment = await client.request('Comment', options.reply) |
| 26 | + const parentVersion = parentComment.version || parentComment.id |
| 27 | + if (parentVersion) replyParent = parentVersion |
| 28 | + if (parentComment.threadRoot) { |
| 29 | + threadRoot = parentComment.threadRoot // <-- BUG: RecordID format |
| 30 | + } else if (parentComment.version) { |
| 31 | + threadRoot = parentComment.version |
| 32 | + } |
| 33 | +} |
| 34 | +``` |
| 35 | + |
| 36 | +The `HMComment` type (from `hm-types.ts`) has: |
| 37 | +- `threadRoot: string` -- a **RecordID** like `z6Mkvz9.../z6Gis...` (authority/tsid) |
| 38 | +- `threadRootVersion: string` -- a **CID** like `bafyreig...` |
| 39 | +- `replyParent: string` -- a **RecordID** |
| 40 | +- `replyParentVersion: string` -- a **CID** |
| 41 | + |
| 42 | +The CLI uses `parentComment.threadRoot` (RecordID) as `rootReplyCommentVersion`, but the downstream code calls `CID.parse()` on it. RecordIDs contain a `/` separator which is not a valid base58btc character, causing the error. |
| 43 | + |
| 44 | +**For a first-level reply** (no threadRoot on the parent), the code falls to `threadRoot = parentComment.version` which IS a CID, so it works. That is why replies to unedited root comments succeed. |
| 45 | + |
| 46 | +**For deeper replies** (where the parent has a threadRoot), the code uses the RecordID format and `CID.parse()` fails. |
| 47 | + |
| 48 | +The edit operation does not change the RecordID or threadRoot of a comment -- it only creates a new version blob with the same TSID. So the real reason editing triggers the bug is likely that the KM agent's two-pass flow (post placeholder -> edit with final answer) creates a scenario where subsequent replies to the edited comment hit the **deeper reply path** (the parent now has threadRoot set because it was itself a reply). |
| 49 | + |
| 50 | +### Problem 2: CID.parse() in createSignedComment (comment.ts lines 306-307) |
| 51 | + |
| 52 | +File: `frontend/packages/client/src/comment.ts` |
| 53 | + |
| 54 | +```typescript |
| 55 | +async function createSignedComment(comment: UnsignedComment, signer: HMSigner): Promise<SignedComment> { |
| 56 | + const commentForSigning = { |
| 57 | + ...comment, |
| 58 | + version: comment.version.split('.').map((v) => CID.parse(v)), |
| 59 | + } as SignedComment |
| 60 | + if (comment.threadRoot) commentForSigning.threadRoot = CID.parse(comment.threadRoot) |
| 61 | + if (comment.replyParent) commentForSigning.replyParent = CID.parse(comment.replyParent) |
| 62 | + // ... |
| 63 | +} |
| 64 | +``` |
| 65 | + |
| 66 | +`CID.parse()` is called on the `threadRoot` and `replyParent` strings. If these are RecordIDs instead of CID strings, the parse fails with the base58btc error. |
| 67 | + |
| 68 | +The same issue exists in `updateComment` (lines 495-496): |
| 69 | +```typescript |
| 70 | +if (input.replyParentVersion) comment.replyParent = CID.parse(input.replyParentVersion) |
| 71 | +if (input.rootReplyCommentVersion) comment.threadRoot = CID.parse(input.rootReplyCommentVersion) |
| 72 | +``` |
| 73 | + |
| 74 | +## How the server works (for reference) |
| 75 | + |
| 76 | +### Comment data model (Go) |
| 77 | + |
| 78 | +File: `backend/blob/blob_comment.go` |
| 79 | + |
| 80 | +```go |
| 81 | +type Comment struct { |
| 82 | + BaseBlob |
| 83 | + ID TSID `refmt:"id,omitempty"` |
| 84 | + Space_ core.Principal `refmt:"space,omitempty"` |
| 85 | + Path string `refmt:"path,omitempty"` |
| 86 | + Version []cid.Cid `refmt:"version,omitempty"` |
| 87 | + ThreadRoot cid.Cid `refmt:"threadRoot,omitempty"` |
| 88 | + ReplyParent_ cid.Cid `refmt:"replyParent,omitempty"` |
| 89 | + Body []CommentBlock `refmt:"body"` |
| 90 | + Visibility Visibility `refmt:"visibility,omitempty"` |
| 91 | +} |
| 92 | +``` |
| 93 | + |
| 94 | +### Comment proto response (Go) |
| 95 | + |
| 96 | +File: `backend/api/documents/v3alpha/comments.go`, function `commentToProto`: |
| 97 | + |
| 98 | +```go |
| 99 | +pb := &documents.Comment{ |
| 100 | + Id: blob.RecordID{Authority: cmt.Signer, TSID: tsid}.String(), // RecordID |
| 101 | + Version: c.String(), // CID (base32 encoded) |
| 102 | + // ... |
| 103 | +} |
| 104 | + |
| 105 | +if cmt.ThreadRoot.Defined() { |
| 106 | + ridRoot, _ := lookup.RecordID(cmt.ThreadRoot) |
| 107 | + ridParent, _ := lookup.RecordID(cmt.ReplyParent()) |
| 108 | + |
| 109 | + pb.ThreadRoot = ridRoot.String() // RecordID format |
| 110 | + pb.ThreadRootVersion = cmt.ThreadRoot.String() // CID format |
| 111 | + pb.ReplyParent = ridParent.String() // RecordID format |
| 112 | + pb.ReplyParentVersion = cmt.ReplyParent().String() // CID format |
| 113 | +} |
| 114 | +``` |
| 115 | + |
| 116 | +Key insight: The server returns BOTH formats -- RecordID (`threadRoot`, `replyParent`) and CID (`threadRootVersion`, `replyParentVersion`). The CLI must use the `*Version` fields (CID) for blob construction, not the RecordID fields. |
| 117 | + |
| 118 | +### CreateComment server handler (Go) |
| 119 | + |
| 120 | +File: `backend/api/documents/v3alpha/comments.go`, function `CreateComment`: |
| 121 | + |
| 122 | +```go |
| 123 | +if in.ReplyParent != "" { |
| 124 | + rpComment, err := srv.getComment(conn, in.ReplyParent) // Accepts RecordID or CID |
| 125 | + replyParent = rpComment.CID // Uses the BLOB CID |
| 126 | + threadRoot = rpComment.Comment.ThreadRoot // Uses the CBOR CID field |
| 127 | + if !threadRoot.Defined() { |
| 128 | + threadRoot = replyParent |
| 129 | + } |
| 130 | +} |
| 131 | +``` |
| 132 | + |
| 133 | +The server's `getComment` resolves comments by RecordID (looking up by authority + TSID, returning the latest version). The server uses the internal CID from the blob, NOT the string IDs. |
| 134 | + |
| 135 | +### Comment edits and version chains |
| 136 | + |
| 137 | +When a comment is edited: |
| 138 | +- A new blob is created with the SAME TSID but different CID |
| 139 | +- The `qGetCommentByID` query returns the latest version (`ORDER BY sb.ts DESC LIMIT 1`) |
| 140 | +- The `version` field in the response changes to the new blob's CID |
| 141 | +- The `id` (RecordID) stays the same |
| 142 | +- Threading fields (threadRoot, replyParent) stay the same (they reference the original blobs) |
| 143 | + |
| 144 | +## The fix |
| 145 | + |
| 146 | +### Fix 1: CLI `comment create` handler |
| 147 | + |
| 148 | +File: `frontend/apps/cli/src/commands/comment.ts` |
| 149 | + |
| 150 | +Change lines 123-135 from: |
| 151 | + |
| 152 | +```typescript |
| 153 | +let replyParent: string | undefined |
| 154 | +let threadRoot: string | undefined |
| 155 | + |
| 156 | +if (options.reply) { |
| 157 | + const parentComment = await client.request('Comment', options.reply) |
| 158 | + const parentVersion = parentComment.version || parentComment.id |
| 159 | + if (parentVersion) replyParent = parentVersion |
| 160 | + if (parentComment.threadRoot) { |
| 161 | + threadRoot = parentComment.threadRoot |
| 162 | + } else if (parentComment.version) { |
| 163 | + threadRoot = parentComment.version |
| 164 | + } |
| 165 | +} |
| 166 | +``` |
| 167 | + |
| 168 | +To: |
| 169 | + |
| 170 | +```typescript |
| 171 | +let replyParent: string | undefined |
| 172 | +let threadRoot: string | undefined |
| 173 | + |
| 174 | +if (options.reply) { |
| 175 | + const parentComment = await client.request('Comment', options.reply) |
| 176 | + // Use the CID version fields, not the RecordID fields. |
| 177 | + // version = CID of the comment blob |
| 178 | + // threadRootVersion = CID of the thread root blob (if this is a reply) |
| 179 | + // replyParentVersion = CID of the reply parent blob (if this is a nested reply) |
| 180 | + const parentVersion = parentComment.version || parentComment.id |
| 181 | + if (parentVersion) replyParent = parentVersion |
| 182 | + if (parentComment.threadRootVersion) { |
| 183 | + threadRoot = parentComment.threadRootVersion // <-- Use CID, not RecordID |
| 184 | + } else if (parentComment.version) { |
| 185 | + threadRoot = parentComment.version |
| 186 | + } |
| 187 | +} |
| 188 | +``` |
| 189 | + |
| 190 | +The key change: `parentComment.threadRoot` -> `parentComment.threadRootVersion` |
| 191 | + |
| 192 | +### Fix 2: Consider also fixing `replyParent` in the CLI `comment edit` handler |
| 193 | + |
| 194 | +File: `frontend/apps/cli/src/commands/comment.ts`, lines 198-213 |
| 195 | + |
| 196 | +The `edit` command already uses `existing.replyParentVersion` and `existing.threadRootVersion` correctly (lines 207-208). Verify this path is correct -- it appears to be. |
| 197 | + |
| 198 | +## Files to modify |
| 199 | + |
| 200 | +1. **`frontend/apps/cli/src/commands/comment.ts`** -- Primary fix: use `threadRootVersion` instead of `threadRoot` in the `create --reply` handler |
| 201 | +2. **`frontend/packages/client/__tests__/comment.test.ts`** -- Add test for `createComment` with reply CID versions |
| 202 | +3. **`frontend/apps/cli/src/test/cli.test.ts`** or **`frontend/apps/cli/src/test/cli-fixture.test.ts`** -- Add integration test for reply-after-edit scenario |
| 203 | + |
| 204 | +## Files to read (for context) |
| 205 | + |
| 206 | +All paths relative to the seed repo root (`/Users/horacioh/seed-hypermedia/seed`). |
| 207 | + |
| 208 | +| File | What to look at | |
| 209 | +|------|-----------------| |
| 210 | +| `frontend/apps/cli/src/commands/comment.ts` | CLI command handlers (create, edit, delete) | |
| 211 | +| `frontend/packages/client/src/comment.ts` | `createComment`, `createSignedComment`, `updateComment`, `CID.parse()` calls | |
| 212 | +| `frontend/packages/client/src/hm-types.ts` | `HMCommentSchema` -- the `threadRoot` vs `threadRootVersion` fields | |
| 213 | +| `backend/api/documents/v3alpha/comments.go` | Server handler: `CreateComment`, `getComment`, `commentToProto` | |
| 214 | +| `backend/blob/blob_comment.go` | `Comment` struct, `NewComment`, `ReplyParent()` fallback logic | |
| 215 | +| `backend/blob/index.go` | `RecordID` type, `DecodeRecordID`, `LookupCache.RecordID` | |
| 216 | +| `backend/blob/tsid.go` | `TSID` type, base58btc encoding | |
| 217 | +| `backend/core/principal.go` | `Principal.String()` (base58btc encoding), `DecodePrincipal` | |
| 218 | + |
| 219 | +## Test plan |
| 220 | + |
| 221 | +### Unit test for the CLI fix |
| 222 | + |
| 223 | +Add to `frontend/packages/client/__tests__/comment.test.ts`: |
| 224 | + |
| 225 | +```typescript |
| 226 | +it('creates a reply comment with threadRoot and replyParent CIDs', async () => { |
| 227 | + const signer = makeSigner() |
| 228 | + // These should be valid CID strings, not RecordIDs |
| 229 | + const threadRootCID = 'bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi' |
| 230 | + const replyParentCID = 'bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi' |
| 231 | + |
| 232 | + const publishInput = await createComment( |
| 233 | + { |
| 234 | + content: makeBlocks('reply text'), |
| 235 | + docId: TEST_DOC_ID, |
| 236 | + docVersion: threadRootCID, |
| 237 | + blobs: [], |
| 238 | + replyCommentVersion: replyParentCID, |
| 239 | + rootReplyCommentVersion: threadRootCID, |
| 240 | + }, |
| 241 | + signer, |
| 242 | + ) |
| 243 | + |
| 244 | + const decoded = cborDecode(publishInput.blobs[0]!.data) as any |
| 245 | + expect(decoded.threadRoot).toBeDefined() |
| 246 | + expect(decoded.replyParent).toBeUndefined() // Same as threadRoot, so omitted |
| 247 | +}) |
| 248 | +``` |
| 249 | + |
| 250 | +### Manual regression test |
| 251 | + |
| 252 | +1. Start a local seed daemon |
| 253 | +2. Create a document |
| 254 | +3. Post comment A on the document |
| 255 | +4. Post comment B with `--reply A` |
| 256 | +5. Edit comment B: `seed-cli comment edit B --body "edited text"` |
| 257 | +6. Post comment C with `--reply B` -- should succeed (currently fails) |
| 258 | +7. Verify comment C has correct `replyParent` and `threadRoot` |
| 259 | +8. Post comment D with `--reply A` (non-edited chain) -- should still work |
| 260 | + |
| 261 | +## Impact on KM agent |
| 262 | + |
| 263 | +Once this fix lands in the seed repo, the KM agent workaround (skipping placeholders for thread-reply triggered comments) can be removed, restoring the two-pass UX (immediate "Working on this..." placeholder followed by the real answer). |
| 264 | + |
| 265 | +The workaround is in the seed-km repo at: |
| 266 | +- `seed-knowledge-manager/agent/mcp/seed-cli-mcp/src/machines/poll-driver.ts` -- placeholder posting logic |
| 267 | +- `seed-knowledge-manager/agent/mcp/seed-cli-mcp/src/tools.ts` -- `seed_reply_comment` tool (line 325) |
| 268 | + |
| 269 | +## CID encoding note |
| 270 | + |
| 271 | +The Go `go-cid` library (v0.6.0) encodes CIDv1 as **base32lower** by default (strings starting with `b`). The JavaScript `multiformats` CID library handles multiple multibase encodings via `CID.parse()`, so base32 CIDs from the server parse correctly. The error only occurs when a non-CID string (RecordID with `/` separator) is passed to `CID.parse()`. |
| 272 | + |
| 273 | +## Run these commands after the fix |
| 274 | + |
| 275 | +```bash |
| 276 | +# From the seed repo root: |
| 277 | + |
| 278 | +# TypeCheck |
| 279 | +pnpm typecheck |
| 280 | + |
| 281 | +# Client package tests |
| 282 | +pnpm --filter @seed-hypermedia/client test |
| 283 | + |
| 284 | +# CLI tests |
| 285 | +pnpm --filter @shm/cli test |
| 286 | + |
| 287 | +# Full test suite |
| 288 | +pnpm test |
| 289 | +``` |
0 commit comments