docs(adr): add ADR 0086 for conversation surface participation - #6083
Conversation
Record the decision to introduce conversation.Client (parallel to tracker.Client) so custom agents can participate in GitHub Discussions and other chat systems without extending forge.Client, and extend NormalizedEvent with conversation category metadata. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · Commit: |
PR Summary by Qododocs: ADR 0086 for conversation surfaces + NormalizedEvent v1 conversation kind
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Site previewPreview: https://64c26e87-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
|
Also declining the ADR line-count skill insight: the domain model table and |
Require has() checks for optional comment.command and category.slug in the NormalizedEvent conversation trigger example so copy-paste triggers do not fail on valid events that omit those fields. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 11:45 AM UTC · Completed 12:01 PM UTC Commit: |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Resolved since last review: The prior Previous run (2)ReviewFindingsMedium
Low
Labels: PR modifies Go production code and tests in internal/normevent/ Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Labels: PR modifies documentation files (ADRs, architecture docs, normative spec, and schema) |
Document that GitHub DiscussionCategory only exposes isAnswerable for format derivation, drop implied announcement/poll API mapping, and address low review nits on is_entity_author and ADR 0076 CEL enforcement. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 6:13 PM UTC · Completed 6:32 PM UTC Commit: |
Resolve architecture.md conflict with upstream, forbid state.conversation on non-conversation entities, clarify GitHub slug/format data sources, update CEL trigger docs, and teach normevent.Event about conversation state so example fixtures round-trip. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Also addressed the earlier review-agent medium findings in f2cbdda:
|
|
🤖 Review · Commit: |
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 9:54 AM UTC · Completed 10:11 AM UTC Commit: |
d6ff96e to
dc083a0
Compare
|
🤖 Review · Commit: |
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 11:22 AM UTC · Completed 11:35 AM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Approving at head ccf2212b. Design review below — nothing blocking; the non-blocking points are follow-on requirements to keep in view when the implementation PRs land.
What this ADR decides (and what it deliberately does not)
The chosen Option D — a narrow conversation.Client parallel to tracker.Client, entity.kind: conversation on NormalizedEvent v1, auth reused from ADR 0054, category ≠ label — is the right seam and consistent with the #5988 split. Note for readers coming from the "plan with an agent in a Discussion, then hand off to /fs-code" use case: this ADR establishes the participation surface only. The Discussion → work-item handoff is explicitly a follow-on (Consequences, last bullet; architecture.md open question). When it lands it goes through tracker.Client (issue create + cross-link) with an issues:write identity delta, not through conversation.Client. Worth keeping that sequencing honest in the implementation tracker.
Design scrutiny (non-blocking)
- Enforcement sequencing. The ADR relies on CEL
triggeroverentity.kind(ADR 0076 target state) to keep code-mutating commands off conversations. Today no harness infullsend-ai/agentscarries atrigger:expression, andinternal/harnessdispatch/project.go(buildEventPayload,triggerSource) only knowswork_item/change_proposal. So the Discussion adapter PR must ship together with (a) entity-kind guards on the coder/fix/review paths and (b) conversation payload projection — otherwise/fs-codetyped in a Discussion would route to the coder with an empty entity payload. Suggest stating that ordering requirement in the implementation issue. - Threading granularity.
transition.commentcarries no parent/thread id. GitHub Discussions have two-level replies, and the ADR maps a Slack thread to a conversation — so a GitHub reply-thread has no analogue and an agent cannot know which sub-thread to answer in. An optionaltransition.comment.parent_id(or thread id) later is an additive, non-breaking schema change; fine to defer, but it will be needed for any real multi-turn session. - Identifier portability.
entity.idis an integer andentity.keyis Jira-shaped (^[A-Za-z][A-Za-z0-9_]+-\d+$). Slack/Discord/Matrix thread ids fit neither, so the portability claim has no identifier carrier yet for non-numeric backends. One-line note for the Slack follow-on. - Identity delta. Confirmed no mint role grants
discussionstoday (internal/mintcore/github.gorole table), so the open question in architecture.md is accurate; aconversation-class role (or per-rolediscussions: write) is a prerequisite for any writeback. - Multi-turn cost/concurrency. With Option B rejected, every message is a full dispatch; two quick replies on the same conversation produce two concurrent runs. Existing per-entity locking/cancel semantics (as for
/fs-fix) should be stated as applying per conversation when the adapter lands.
Open bot findings at head — neither is major: the review-agent Medium about the docs(adr): title vs. production Go/schema changes is fair (consider feat(normevent): … before merge); the Low about buildEventPayload is the same latent gap as point 1.
Align ADR 0086 with Slack conversation/thread naming and GitHub discussion_comment parent pointers. Require comment.id and parent_id (parent_id == id on thread roots) instead of state.conversation.thread. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 9:08 PM UTC · Completed 9:26 PM UTC Commit: |
After rebasing on main, Comment.ID changed from int to string per ADR 0086 (conversation support via #6083). Update all code paths: - ghaevent.go: convert intField result to string with strconv.Itoa - project.go: check Comment.ID != "" instead of != 0 - project_test.go, ghaevent_test.go: update test assertions to expect string - cleanup_test.go: add ListIssueReactions stub to fakeRetryCleanupSCM Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
After rebasing on main, Comment.ID changed from int to string per ADR 0086 (conversation support via #6083). Update all code paths: - ghaevent.go: convert intField result to string with strconv.Itoa - project.go: check Comment.ID != "" instead of != 0 - project_test.go, ghaevent_test.go: update test assertions to expect string - cleanup_test.go: add ListIssueReactions stub to fakeRetryCleanupSCM Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Summary
conversation.Client(parallel totracker.Client), so agents can participate in GitHub Discussions and later other chat systems without growingforge.Client.NormalizedEventv1 withentity.kind: conversationand requiredstate.conversation.category(1:M category→conversation; M:M labels on conversations only; messages inherit parent context).docs/architecture.mdand cross-links ADR 0076.Test plan
make linton staged ADR/docs changesconversation.Clientseam againstinternal/trackerMade with Cursor