feat(comments): unified comment system + plan review command

[birdmanmandbir]

Summary

• New ttal comment add/list — unified comment command backed by ent SQLite comments table; auto-resolves current task from TTAL_JOB_ID (workers) or TTAL_AGENT_NAME (managers); notifies counterpart via tmux
• Remove ttal task comment and ttal pr comment — all commenting now goes through ttal comment
• LGTM via +lgtm tag — reviewers run task <uuid> modify +lgtm; on-modify hook enforces TTAL_ROLE=reviewer guard; ParsePRID silently strips legacy :lgtm suffix
• New ttal plan review <uuid> — spawns plan-reviewer tmux window (or triggers re-review if already running); backed by internal/planreview package mirroring internal/review
• Updated all agent/skill/doc/template references to new commands

Test plan

• make ci passes (confirmed before PR)
• ttal comment add "msg" works in a worker session (resolves via TTAL_JOB_ID)
• ttal comment list shows task comments
• task <uuid> modify +lgtm blocked outside reviewer role
• ttal plan review <uuid> spawns plan-review window

[birdmanmandbir]

PR Review — ttal comment system + LGTM tag gate

Summary

Solid architectural improvement. The comment DB unification and LGTM tag gate are clean designs. The main concerns are silent failure modes and missing test coverage for new security-critical logic.

---

Important Issues

1. Three forge side-effect errors swallowed silently — cmd/comment_new.go:110-129

Three of the four error paths in the forge side-effect block are silently dropped with no user feedback. Only prErr is warned. If ExportTask, ResolveContextWithoutProvider, or PRIndex fail, the user gets "Comment added (round N)" but the forge PR never gets the comment — with zero indication why.

// These failures are currently silent:
task, taskErr := taskwarrior.ExportTask(taskUUID)   // taskErr only checked as guard
prCtx, ctxErr := pr.ResolveContextWithoutProvider() // ctxErr swallowed
idx, idxErr := pr.PRIndex(prCtx)                    // idxErr swallowed

Fix: emit "warning: forge comment skipped: ..." to stderr for each failure, consistent with the existing prErr warning pattern.

---

2. All tmux notification errors discarded in notifyCounterpart() — cmd/comment_new.go:197-247

All four tmux.SendKeys / review.RequestReReview calls use _ =. In a multi-agent system, stalling notifications have no trace in logs or output. Also writeReviewFile, _ := at line 214 silently suppresses the review file reference in the triage notification.

Fix: Best-effort notification is fine, but failures should log.Printf("warning: ...") so agents can diagnose stalled workflows.

---

3. resolveCurrentTask conflates taskwarrior error with empty-list — cmd/comment_new.go:38-39

if err != nil || len(tasks) == 0 {
    return "", fmt.Errorf("no active task with +%s tag", agent)
}

If ExportTasksByFilter fails (tw binary missing, TASKRC broken, timeout), the message says "no active task" — points the user at task state instead of infrastructure.

Fix: return "", fmt.Errorf("taskwarrior query failed: %w", err) for the error case; keep the existing message for the empty-list case.

---

4. slices.Contains used directly instead of ctx.Task.HasTag() — cmd/pr.go:184, internal/pr/pr.go:92

The Task.HasTag() method already exists and the PR adds a package-level HasTag() function too. But the two new LGTM check call sites bypass both and use slices.Contains directly.

// current (two new sites):
if !slices.Contains(ctx.Task.Tags, "lgtm") {

// preferred:
if !ctx.Task.HasTag("lgtm") {

---

5. CurrentRound full table scan instead of MAX aggregate — internal/comment/service.go:48-62

Fetches all comments ordered by round and reads the last element. A full table scan per ttal comment add. Use .Aggregate(ent.Max(...)) or .Order(DESC).Limit(1) instead. Also has a TOCTOU window under concurrent comment creation.

---

6. Missing tests for checkLGTMGuard — internal/worker/hook_on_modify.go:72-82

This is the enforcement point for the new security model. It's a pure function with no I/O except os.Getenv — trivially testable with t.Setenv. Missing cases:

• +lgtm added with TTAL_ROLE=reviewer → allow
• +lgtm added with TTAL_ROLE=coder → reject
• +lgtm added with TTAL_ROLE="" → reject (empty role path)
• +lgtm already in original tags (not being newly added) → allow

The empty-role path is the most dangerous unverified path — if the condition were accidentally inverted, the gate would be open to everyone.

---

7. Missing tests for comment.Service — internal/comment/service.go

Add, List, and CurrentRound have zero tests. The round increment behavior (first comment → round 1, second → round 2, cross-target isolation) is not verified. enttest.Open with sqlite3 file::memory: works cleanly for this.

---

Suggestions

• CommentAdd/CommentList client functions (internal/daemon/socket.go:612-654): duplicate the marshal→POST→decode→error-check pattern already generalised by prCallTyped. Could reuse the existing generic helper.
• checkLGTMGuard guard clause style: the project's own CLAUDE.md calls out guard clauses as a preference. The inner nesting can be flattened — if !lgtmAdded { return nil } / if role == "reviewer" { return nil } / return fmt.Errorf(...).
• json.Marshal(original) error discarded (hook_on_modify.go:99): if marshal somehow fails, taskwarrior receives empty stdout and may corrupt the task. A defensive if len(rawOriginal) == 0 { os.Exit(1) } guard prevents that.
• "7:lgtm" in TestValidateTaskCompletion_PRMergedWithLGTM: now tests a legacy data format. Use "7" to reflect current production data path.
• ParsePRID backward compat: the "123:other" error case was deleted from prid_test.go without replacement — it still errors correctly but is no longer verified.

---

Strengths

• LGTM gate migration is clean — +lgtm tag is a better contract than embedded PRID suffix
• internal/pr/pr_test.go TestMergeLGTMGate updated correctly with mock Tags
• comment.Service and comment_handler.go are well-structured; schema is clean with correct immutable created_at
• notifyCounterpart correctly handles 4 roles with distinct semantic actions
• SpawnPlanReviewer cleanup path is correct (removes session file on NewWindow failure)
• doc.go plane annotations present for both new packages

---

VERDICT: NEEDS_WORK

Fix the two silent failure issues (#1, #2) and add checkLGTMGuard tests (#6) before merge. Issues #3–5 and #7 can be follow-ups.

[birdmanmandbir]

Triage Update

Fixed

• Silent forge side-effect errors — all 4 paths now emit warning: forge comment skipped: ... to stderr (bb29ac0)
• tmux notification errors discarded — extracted 4 helper functions (notifyReviewer, notifyCoder, notifyPlanReviewer, notifyDesigner), all errors log via log.Printf; also fixed writeReviewFile error suppression (bb29ac0)
• resolveCurrentTask error conflation — taskwarrior query errors now "taskwarrior query failed: %w", empty list stays "no active task with +%s tag" (bb29ac0)
• slices.Contains replaced with ctx.Task.HasTag("lgtm") at both sites (cmd/pr.go:184, internal/pr/pr.go:92); removed unused "slices" imports (bb29ac0)
• CurrentRound full table scan — now uses Limit(1) + ByRound(sql.OrderDesc()) (bb29ac0)
• checkLGTMGuard tests added — reviewer/coder/empty-role/already-set/unrelated cases in hook_on_modify_test.go (bb29ac0)
• comment.Service tests added — round increment, cross-target isolation, list ordering, empty round in internal/comment/service_test.go (bb29ac0)
• Guard clause style for checkLGTMGuard — two early returns before the error (bb29ac0)
• json.Marshal defensive guard — exits with code 1 if marshal fails or returns empty (bb29ac0)
• "7:lgtm" → "7" in TestValidateTaskCompletion tests (bb29ac0)
• "123:other" error case added to prid_test.go (bb29ac0)

No Remaining Issues

[birdmanmandbir]

Re-review — bb29ac0

Fixed ✓

• Silent forge side-effect errors — all 4 paths now emit warning: forge comment skipped: ... to stderr; else if task.PRID != "" chain is clear and explicit
• tmux notification errors discarded — extracted into notifyReviewer, notifyCoder, notifyPlanReviewer, notifyDesigner; all errors logged via log.Printf; writeReviewFile error now logged too
• resolveCurrentTask error conflation — split into "taskwarrior query failed: %w" vs "no active task with +%s tag" correctly
• slices.Contains → ctx.Task.HasTag("lgtm") at both call sites; unused "slices" imports removed
• CurrentRound full table scan — now ORDER BY round DESC LIMIT 1, returns comments[0].Round; clean
• checkLGTMGuard tests — 5 table-driven cases: reviewer/coder/empty-role/already-set/unrelated; all required paths covered
• comment.Service tests — round increment, cross-target isolation, list ordering, empty-returns-zero; in-memory SQLite setup is correct
• Guard clause style — two early returns before the error path; matches CLAUDE.md preference
• json.Marshal defensive guard — marshalErr != nil || len(rawOriginal) == 0 → os.Exit(1)
• "7:lgtm" → "7" in both TestValidateTaskCompletion cases
• "123:other" error case restored in prid_test.go

No Remaining Issues

All critical, important, and suggestion-level items from the original review have been addressed. The fix commit is thorough and complete.

VERDICT: LGTM