Skip to content

refactor(#6767): route status notifications through tracker.Client - #6770

Open
fullsend-ai-coder[bot] wants to merge 4 commits into
mainfrom
agent/6767-tracker-status-routing
Open

refactor(#6767): route status notifications through tracker.Client#6770
fullsend-ai-coder[bot] wants to merge 4 commits into
mainfrom
agent/6767-tracker-status-routing

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

Route run-status notifications (comments, reactions, orphan reconciliation) through tracker.Client instead of forge.Client, so the notification destination is determined by event provenance rather than the code-output forge.

  • Extend tracker.Client with DeleteComment; add tracker.Reactor as an optional interface for emoji reactions
  • Refactor statuscomment.Notifier and ReconcileOrphaned to use tracker.Client with (project, number) addressing
  • Update all callers (setupStatusNotifierGitHub, setupStatusNotifierGitLab, reconcile-status) to wrap forge.Client in tracker.NewForgeClient()
  • Add jira.LiveClient.DeleteComment for Jira comment deletion support
  • Write ADR 0093 recording the decision to route by event provenance

Changes

  • internal/tracker/tracker.go — Add DeleteComment to Client, add Reactor interface
  • internal/tracker/forge_client.go — Implement DeleteComment and all Reactor methods
  • internal/tracker/jira_client.go — Implement DeleteComment
  • internal/tracker/fake_jira.go — Add DeleteComment to test fake
  • internal/forge/jira/client.go — Add LiveClient.DeleteComment
  • internal/statuscomment/statuscomment.go — Replace forge.Client with tracker.Client; reactions via optional Reactor type assertion
  • internal/cli/run.go — Wrap forge clients in tracker.NewForgeClient(); ClientFactory returns tracker.Client
  • internal/cli/reconcilestatus.go — Wrap forge client for ReconcileOrphaned
  • docs/ADRs/0093-tracker-routed-status-notifications.md — New ADR
  • docs/architecture.md — Cross-reference ADR 0093

Testing

  • All existing statuscomment tests updated and passing (94% coverage)
  • All existing tracker tests passing
  • All reconcile-status CLI tests updated and passing
  • New test: TestNotifier_ReactionsSkippedForNonReactorTracker — verifies Jira-backed trackers silently skip reactions
  • go vet, gofmt, ADR lints, link checks pass

Closes #6767

Post-script verification

  • Branch is not main/master (agent/6767-tracker-status-routing)
  • Secret scan passed (gitleaks — 0195363fd5399191a2df4f0f66b9a18e9c25a903..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

Status comments and reactions were coupled to forge.Client with
owner/repo/number addressing. When a Jira issue triggers a run,
the Jira issue number was passed to the forge as a GitHub issue
number, producing a 404 because no such GitHub issue exists.

Refactor statuscomment.Notifier and ReconcileOrphaned to accept
tracker.Client instead of forge.Client. This routes status
notifications to whichever tracker originated the run (GitHub,
GitLab, or Jira) independently of the forge used for code output.

Key changes:
- Add DeleteComment to tracker.Client for start-comment cleanup
- Add tracker.Reactor interface for optional emoji reactions;
  ForgeClient implements it, JiraClient does not
- Notifier uses (project, number) addressing instead of
  (owner, repo, number) to match tracker.Client's model
- ClientFactory returns tracker.Client instead of forge.Client
- SetTriggerCommentID accepts string (tracker comment IDs are
  strings for JSON round-tripping safety)
- Callers wrap forge.Client in tracker.NewForgeClient()
- Add jira.LiveClient.DeleteComment for Jira comment deletion
- Add ADR 0093 recording the provenance-based routing decision

Pre-commit hooks could not be run in-sandbox (network-restricted);
gofmt, go vet, ADR lints, and link checks were run directly.

Closes #6767
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown

Site preview

Preview: https://30193f52-site.fullsend-ai.workers.dev

Commit: dd044f19ea5508f6991d32fbb78b816435f071d2

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:35 PM UTC · Completed 8:51 PM UTC

Commit: 634d127 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.72

@codecov

codecov Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.56604% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/statuscomment/statuscomment.go 79.59% 4 Missing and 6 partials ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Aug 29, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Medium-sized refactor (1138 lines, 15 files) routing status notifications through tracker.Client; zero protected paths, zero security-sensitive files, no CI or dependency changes, bot author; elevated git history churn on core files (run.go, statuscomment.go) but well-scoped refactor aligned with linked issue #6767; score unchanged from prior assessment as signals remain stable.

Previous run

Risk Assessment: moderate (2/5)

Details

Medium-sized refactor (1093 lines, 15 files) routing status notifications through tracker.Client; zero protected paths, zero security-sensitive files, no CI or dependency changes, bot author; elevated git history churn on core files (run.go, statuscomment.go) but well-scoped refactor aligned with linked issue #6767; score unchanged from prior assessment as signals remain stable.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Medium-sized refactor (827 lines, 13 files) routing status notifications through tracker.Client; zero protected paths, zero security-sensitive files, no CI or dependency changes, bot author; elevated git history churn on core files (statuscomment.go, run.go) but well-scoped refactor aligned with linked issue #6767.

Previous run (3)

Risk Assessment: moderate (2/5)

Details

Medium-sized refactor (827 lines, 13 files) routing status notifications through tracker.Client; zero protected paths, zero security-sensitive files, no CI or dependency changes, bot author; elevated git history churn on core files (statuscomment.go, run.go) but well-scoped refactor aligned with linked issue #6767.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [stale-doc] docs/guides/user/jira-integration.md:36 — The statements at lines 36 and 69 claiming "the built-in agent pipeline does not use [tracker.Client] yet" are now stale. This PR migrates the statuscomment package from forge.Client to tracker.Client, making tracker.Client a consumer in the built-in agent pipeline. While dynamic event-source routing to Jira is a declared follow-on (ADR 0093), the claim that the pipeline does not use tracker.Client is no longer accurate. The practical user-facing behavior is unchanged — Jira-triggered runs still do not post status back to Jira — but the internal plumbing description is now inaccurate.
Previous run

Looks good to me

Previous run (2)

Review

Findings

Low

  • [comment-hygiene] internal/statuscomment/statuscomment_test.go — The diff removes ~30 inline test comments. Most are low-value restatements of the next line of code, but a few (~4–5) explained non-obvious edge-case reasoning (e.g., why wasSkipped forces synthesis, why no synthesis when completionMode isn’t on_failure, why job-succeeded means no synthesis). These were the same comments flagged in the prior round and remain removed.
Previous run (3)

Review

Findings

Low

  • [stale-documentation] internal/tracker/tracker.go:15 — The package doc comment says "Nothing calls tracker.Client yet." After this PR merges, statuscomment.Notifier and ReconcileOrphaned both call tracker.Client, making this statement false.
    Remediation: Update line 15 to reflect that statuscomment is now a consumer of tracker.Client.

  • [comment-hygiene] internal/statuscomment/statuscomment_test.go — The diff removes ~30 inline test comments. Most are low-value restatements of the next line of code, but a few (~4–5) explained non-obvious edge-case reasoning (e.g., why the synthesized label is outcome-neutral, why script-controlled text must not forge markers) and genuinely aided comprehension. The majority of removals are justified cleanup from the API surface change.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 8:53 PM UTC · Completed 9:01 PM UTC

Commit: 634d127 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.70

- Update stale package doc in internal/tracker/tracker.go: replace
  "Nothing calls tracker.Client yet" with current consumers
  (statuscomment and reconcilestatus)
- Restore five test comments that explain non-obvious edge-case
  reasoning removed during the API surface refactoring: outcome-neutral
  synthesized label (#5736), script-controlled text must not forge
  markers, reaction consistency on comment failure, paragraph break
  for GitLab CommonMark, and crash-before-post synthesis (#3635)

Addresses review feedback on #6770
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (bot-triggered)

Addressed both review findings. Updated stale package doc in tracker.go and restored 5 valuable test comments that explain non-obvious edge-case reasoning.

Fixed (2):

  1. stale-documentation in internal/tracker/tracker.go:15 (internal/tracker/tracker.go): Updated package doc comment from 'Nothing calls tracker.Client yet' to list current consumers: statuscomment (run-status notifications) and reconcilestatus (orphan cleanup)
  2. comment-hygiene in internal/statuscomment/statuscomment_test.go (internal/statuscomment/statuscomment_test.go): Restored five test comments that explain non-obvious edge-case reasoning: (1) why the synthesized label is outcome-neutral (PR feat(#3697): add on_failure mode for comment.completion status notifications #5736), (2) why script-controlled text must not forge markers, (3) why the start reaction must survive a failed completion comment, (4) why paragraph breaks are needed for GitLab CommonMark rendering, (5) why synthesis is needed when a crash occurs before any comment is posted (Post failure status comment when agent fails before creating a status comment #3635)

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 29, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:02 PM UTC · Completed 9:17 PM UTC

Commit: 9c065bc · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.27

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Aug 29, 2026
@ralphbean

Copy link
Copy Markdown
Member

/fs-fix improve code coverage to satisfy codecov

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 3:32 PM UTC · Completed 3:41 PM UTC

Commit: 9c065bc · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.57

Cover ForgeClient.DeleteComment, AddIssueReaction, DeleteIssueReaction,
AddCommentReaction, DeleteCommentReaction; JiraClient.DeleteComment;
FakeJiraClient via NewFakeJiraClient; and jira.LiveClient.DeleteComment.
Raises tracker package coverage from 67% to 92%, satisfying the 80%
codecov/patch threshold.

Addresses review feedback on #6770
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (human-triggered)

Added 20 test functions across 3 test files to cover all new methods introduced in this PR. Tracker package coverage rose from 67% to 92%, jira package from 89% to 89%, all new functions now at 100%. All tests pass, go vet clean, gofmt clean.

Fixed (4):

  1. codecov/patch coverage below 80% threshold (internal/tracker/forge_client.go): Added unit tests for ForgeClient.DeleteComment, AddIssueReaction, DeleteIssueReaction, AddCommentReaction, and DeleteCommentReaction — all new methods added in this PR that had 0% coverage. Tests cover success paths, invalid project errors, and invalid comment ID errors.
  2. codecov/patch coverage below 80% for JiraClient.DeleteComment (internal/tracker/jira_client.go): Added unit tests for JiraClient.DeleteComment covering both the success path (create then delete) and the not-found path (deleting a nonexistent comment wraps to tracker.IsNotFound).
  3. codecov/patch coverage below 80% for FakeJiraClient.DeleteComment and NewFakeJiraClient (internal/tracker/fake_jira.go): Added TestNewFakeJiraClient exercising the constructor and create-comment round-trip. FakeJiraClient.DeleteComment is now covered transitively by the JiraClient.DeleteComment tests.
  4. codecov/patch coverage below 80% for jira.LiveClient.DeleteComment (internal/forge/jira/client.go): Added TestDeleteComment and TestDeleteComment_NotFound using httptest server to exercise the new LiveClient.DeleteComment method, covering the success (204) and not-found (404) paths.

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 30, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:43 PM UTC · Completed 3:57 PM UTC

Commit: 279cf28 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.26

fullsend-ai-review[bot]

This comment was marked as outdated.

@ralphbean

Copy link
Copy Markdown
Member

/fs-fix two corrections - for one, JIRA cloud actually does support reactions for comments, but not for issues. Clarify this in the ADR. It can still be optional for now. Second thing to fix -> the event source wiring (dynamic routing) is not emphasized in the ADR like I think it should be. Switching from the forge client to the tracker client is a small potatoes internal refactoring detail. The idea that we dynamically choose which tracker to use based on the event is a bigger idea though. Update the ADR to make that clear. THe move to the tracker interface away from the forge interface is a side effect of this more central idea. Once you update the ADR, make sure to reflect the new structure to the rest of the PR.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 6:36 PM UTC · Completed 6:46 PM UTC

Commit: 279cf28 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.48

…t in ADR 0093

Restructure ADR 0093 to lead with dynamic event-source routing as the
central architectural idea — the notification destination is determined
by which tracker originated the event, not by the code-output forge.
The forge.Client → tracker.Client refactoring is presented as a
consequence of this routing decision rather than the headline.

Correct the claim that Jira has no emoji-reaction support: Jira Cloud
supports reactions on comments but not on issues. The Reactor interface
remains optional because Jira's partial support doesn't fit the
interface which includes issue-level reactions.

Update architecture.md cross-reference, tracker.Reactor doc comment,
statuscomment package/Notifier docs, and test comments to reflect
the new framing consistently.

Addresses #6770
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (human-triggered)

Restructured ADR 0093 to emphasize dynamic event-source routing as the central idea (not the forge-to-tracker interface refactoring). Corrected Jira reaction claims across ADR, tracker.Reactor doc, statuscomment docs, and test comments. Updated architecture.md cross-reference to match.

Fixed (2):

  1. Jira Cloud actually supports reactions for comments but not for issues (docs/ADRs/0093-tracker-routed-status-notifications.md): Corrected the ADR's claim that 'Jira has no equivalent' for emoji reactions. Jira Cloud supports comment reactions but not issue reactions. The Reactor interface remains optional because Jira's partial support doesn't cleanly fit the interface which includes issue-level reactions. Updated the same clarification in tracker.Reactor doc comment, statuscomment test comment, and architecture.md cross-reference.
  2. Event-source routing (dynamic routing) is not emphasized as the central idea in the ADR (docs/ADRs/0093-tracker-routed-status-notifications.md): Restructured the ADR to lead with dynamic event-source routing as the central architectural concept. The Context section now opens with the routing problem (notifications must go back to the originating tracker). Added a new 'Event-source wiring' section under Decision showing how callers select the tracker adapter based on event source. Reframed the forge.Client to tracker.Client refactoring as a consequence of the routing decision, not the headline. Removed the separate 'Call-site wiring' section (merged into Event-source wiring). Updated Consequences to lead with 'Dynamic routing' as the primary outcome. Reflected the new framing in architecture.md, statuscomment package doc, and Notifier doc comment.

Tests: passed

Decision points
  • How to present Jira's partial reaction support (alternatives: Mark Jira as having full reaction support and implement Reactor, Acknowledge partial support but keep Reactor optional for now; rationale: Human instruction said 'it can still be optional for now'. Jira Cloud supports comment reactions but not issue reactions, and the Reactor interface includes issue-level reactions, so partial implementation would be misleading. Documented the gap and noted adding Jira comment-reaction support is straightforward once needed.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 31, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:48 PM UTC · Completed 7:04 PM UTC

Commit: dd044f1 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge ready-for-review Agent PR ready for human review risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Route run status notifications to the source issue tracker

1 participant