refactor(#5988): introduce tracker.Client interface with forge adapter - #5993
Conversation
Adds internal/tracker, a narrower interface for issue-content read/write (title, body, comments) keyed by (project string, number int) instead of (owner, repo string, number int). This lets Jira, whose issues are keyed as PROJECT-123, implement the same interface later without a forced owner/repo split. forge.Client stays scoped to git-hosting operations. ForgeClient adapts any forge.Client (GitHub or GitLab) to tracker.Client by splitting the project string back into owner/repo. No behavior change: nothing calls tracker.Client yet. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
PR Summary by QodoAdd tracker.Client interface and ForgeClient adapter
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
waynesun09
left a comment
There was a problem hiding this comment.
Review squad pass (Claude + Grok, cross-verified against issue #5989 and the existing internal/forge/gitlab/internal/forge/jira implementations). Posting the Medium+ findings that aren't already covered by the existing qodo-code-review comments on this PR (the "invalid project accepted" and "Jira key docs inconsistent" threads already capture two other issues we found independently and are not re-posted here).
One more Medium finding with no single line to anchor to: per this repo's COMMITS.md, adding an internal interface with no user-visible behavior change (the PR body says "nothing calls tracker.Client yet") should be refactor(#5988): ..., not feat(#5988): ... — feat titles surface directly in release notes under Features.
Jira's comment update endpoint (PUT /issue/{issueIdOrKey}/comment/{commentId})
needs the issue key, not just a comment ID, unlike GitHub/GitLab where a
comment ID alone is enough. Add number to UpdateComment so a future Jira
tracker.Client can reconstruct PROJECT-123; ForgeClient ignores it since
forge.Client.UpdateIssueComment doesn't need it.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
splitProject now returns an error for malformed project strings
("project", "/repo", "owner/") instead of silently forwarding an
empty owner or repo into forge.Client calls. Also fixes two doc
comments in tracker.go: the package doc described a full Jira issue
key ("PROJECT-123") as the project string, contradicting the
interface's separate number param; and the Comment.ID doc claimed
UpdateComment could delete a comment (there's no DeleteComment) and
overstated that comment IDs are non-numeric across trackers.
Addresses review feedback from qodo-code-review and waynesun09 on
PR #5993.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
|
Same two findings as the inline review comments — fixed both in 70a15d6. Referencing this comment: #5993 (comment) |
…ract The Client interface previously said nothing about error semantics, so a future consumer coded against tracker.Client alone had no forge-agnostic way to detect "not found" without reaching into internal/forge directly -- defeating the point of the abstraction. Add tracker.ErrNotFound / IsNotFound, document that implementations must satisfy it, and have ForgeClient translate forge.ErrNotFound into it via a small wrapNotFound helper applied to all four methods. Addresses review feedback from waynesun09 on PR #5993. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
waynesun09
left a comment
There was a problem hiding this comment.
Approving
Clean, well-scoped interface + adapter. Re-reviewed the current state and verified independently — built, go vet-ed, and ran go test ./internal/tracker/... on the branch (Go 1.26): all green.
- Prior review findings resolved:
UpdateCommentnow carriesnumber int(with a clear doc note that forge ignores it but Jira needs it), and theErrNotFound/IsNotFoundcontract is defined and documented on the interface. splitProjectcorrectly last-/-splits so GitHubowner/repoand nested GitLabgroup/subgroup/projectboth work, and it rejects empty owner/repo rather than forwarding malformed values — covered by tests.wrapNotFounduses double-%wso the returned error satisfies bothtracker.ErrNotFoundand the underlyingforge.ErrNotFound, verified byTestForgeClient_GetIssue_NotFound.- Compile-time interface assertions (
var _ Client = …) and TDD coverage across happy-path, nested-namespace, not-found, create/list, update, and invalid-ID.
One tiny optional nit, non-blocking: CreateComment dereferences the *forge.IssueComment without a nil check — only a theoretical panic if a forge.Client impl ever returned (nil, nil) on success, which none do today. Fine to leave.
LGTM.
|
🤖 Finished Review · ✅ Success · Started 2:55 PM UTC · Completed 3:12 PM UTC Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Looks good to me Previous runLooks good to me Previous run (2)ReviewFindingsHigh
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Low
Next steps:
|
|
🤖 Finished Fix · ✅ Success · Started 3:13 PM UTC · Completed 3:19 PM UTC Commit: |
Condense UpdateComment doc comment from 5 lines to 2 lines, matching the compact style of the other Client interface methods while retaining the key rationale (Jira requires the issue key to update a comment). PR title convention (feat → refactor) noted but requires manual change since sandbox cannot mutate the PR title. Addresses review feedback on #5993
🔧 Fix agent — iteration 1 (bot-triggered)Addressed both review findings. (1) PR title convention: verified feat should be refactor per COMMITS.md; sandbox cannot edit the PR title -- requires manual change by maintainer. (2) UpdateComment doc comment: condensed from 5 lines to 2 lines, matching the compact style of the other Client interface methods. Fixed (2):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 3:21 PM UTC · Completed 3:36 PM UTC Commit: |
|
🤖 Finished Fix · ✅ Success · Started 3:37 PM UTC · Completed 3:45 PM UTC Commit: |
Add doc comments to GetIssue, ListComments, and CreateComment in the Client interface and ForgeClient methods to match the existing UpdateComment documentation style. Addresses review feedback on #5993
🔧 Fix agent — iteration 2 (bot-triggered)Addressed 3 of 4 findings. Fixed doc-style inconsistency by adding doc comments to undocumented interface methods and expanding ForgeClient method comments. PR title change (feat->refactor) was attempted but blocked by sandbox network policy -- requires manual change. Disagreed with ADR suggestion as out of scope. Fixed (3):
Disagreed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 3:46 PM UTC · Completed 4:01 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass focused on two Medium findings not covered by existing threads on this PR: duplicated owner/repo-splitting logic between internal/tracker and internal/poll, and a NotFound-contract test-coverage gap across three of Client's four methods.
The comment claimed GitLab comment IDs are globally unique like GitHub's, but GitLab's Notes API actually requires the issue/MR IID to address a note directly (see gitlab.LiveClient.updateOrDeleteNote's scan-based workaround). forge.Client's UpdateIssueComment doesn't expose that IID, so ForgeClient's GitLab path still hits the scan; the doc now says so instead of implying GitLab doesn't need the number at all. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…racker NotFound tests FakeClient.UpdateIssueComment returned nil for an unknown comment ID instead of forge.ErrNotFound, which meant nothing exercised ForgeClient.UpdateComment's wrapNotFound path through the fake. Also add the missing NotFound-contract tests for ListComments, CreateComment, and UpdateComment (only GetIssue had one). Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
Re: #5993 (comment) — the feat-vs-refactor point is fair, this is an internal abstraction with no user-visible behavior yet. Retitled to The other two items in there (UpdateComment's |
|
🤖 Finished Review · ✅ Success · Started 3:04 PM UTC · Completed 3:18 PM UTC Commit: |
Superseded by updated review
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 2 additional findings not covered by existing review threads.
forge.ErrNotFound's own text is "not found", so wrapping it with
fmt.Errorf("%w: %w", ErrNotFound, err) produced doubled text like
"not found: not found" or "not found: not found: comment 42" whenever
a ForgeClient method's Error() was rendered directly (CLI output, logs).
errors.Is/IsNotFound checks were unaffected, but the surfaced message
was wrong.
notFoundError wraps the forge error without repeating the sentinel
text — its Error() returns the forge error's message verbatim, while
Unwrap() []error still satisfies both tracker.IsNotFound and
forge.IsNotFound.
Reported by waynesun09 on PR #5993.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
waynesun09 flagged that a plain string Body doesn't account for Jira's
ADF requirement — internal/forge/jira already models bodies as `any`
for exactly this reason.
Confirmed with a live spike against Jira's v3 API: posting a bare
string as a comment body is rejected outright ("Comment body is not
valid!"). Naively wrapping the raw Markdown string in a single ADF
text node is worse than lossy — Jira's plain-text rendering path
interprets stray Markdown characters as wiki-markup, so braces in a Go
code sample broke the surrounding paragraph and a Markdown link got
mangled into a dead in-page anchor. A properly structured ADF payload
(codeBlock node, link mark, bulletList) rendered correctly.
tracker.Body documents this contract so a future Jira Client
implementation is responsible for real Markdown<->ADF conversion,
rather than callers or the interface silently assuming a pass-through
works.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 4:08 PM UTC · Completed 4:21 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Review squad follow-up: one additional Medium finding not covered by existing threads on this PR.
|
🤖 Finished Retro · ✅ Success · Started 5:41 PM UTC · Completed 6:10 PM UTC Commit: |
Retro: PR #5993 — tracker.Client interface with forge adapterPR: #5993 — What went well
What the review agent missedThe human reviewer (waynesun09) found 8 substantive findings across 4 review passes. The review agent missed all of the interface design and adapter pattern findings despite extensive exploration (the correctness subagent ran 12 Grep searches and multiple file reads across the forge package in run 4 alone):
Review-fix loop (3 iterations, Aug 7)The review-fix loop ran 3 fix iterations over ~1.5 hours. The review agent found a valid HIGH finding (commit convention) but the fix agent couldn't change the PR title due to sandbox network restrictions (403 on Existing issues cover this pattern: #902 (circuit breaker), #2959 (dedup findings across iterations), #2418 (fix agent PR metadata), #685 (persist prior findings on re-review). Redundant dispatches30 shim runs fired on this PR branch: 5 dispatched review, 3 dispatched fix, 1 dispatched retro, 21 were no-ops (pull_request_review events matching no dispatch stage, or cancelled by concurrency groups). Heavily covered by 20+ existing issues (#893, #963, #1271, #2994, #4681, #5967). Existing issues covering other observations
Autonomy readinessThe review agent is not ready for increased autonomy on PRs introducing new Go interface adapters. The human reviewer caught 6+ findings the agent missed, including error wrapping quality, backend-specific type design, cross-package duplication, and test coverage gaps. The agent's unique contributions beyond what the human found were LOW-severity procedural items (doc formatting, ADR suggestion). Human review remains essential for this class of change. Proposals filed |
Summary
internal/tracker, a narrower interface for issue-content read/write (title, body, comments) keyed by(project string, number int)instead of(owner, repo string, number int). This lets a future Jira implementation use its naturalPROJECT-123key instead of a forced owner/repo split.ForgeClientadapts anyforge.Client(GitHub or GitLab) totracker.Clientby splittingprojectback into owner/repo — sinceforge.Clientalready abstracts GitHub vs GitLab, one adapter covers both.tracker.Clientyet.UpdateCommenttakes an explicitnumber intparameter in addition tocommentID, which isn't in issue Introduce tracker.Client: a forge-agnostic issue-content interface for GitHub/GitLab #5988's original spec. This is a deliberate deviation: Jira's REST API needs the issue key (project+number) to update a comment, not just a comment ID, so the interface has to carrynumberhere even though today's GitHub/GitLab adapter ignores it.FakeClient.UpdateIssueCommentis changed from always returning nil to returningfmt.Errorf("%w: comment %d", ErrNotFound, commentID)when the comment ID isn't found.Closes #5988. Step toward #5989 (Jira
tracker.Clientimplementation) and #5991 (fullsend issues get/post-commentCLI).Test plan
go test ./internal/tracker/...(new tests, TDD: written first and confirmed failing before implementation)go build ./...go vet ./internal/tracker/...make lintgo test ./...(pre-existing unrelated failure ininternal/scaffoldTestFileModeMatchesFilesystem, confirmed present onmainbefore this change)🤖 Generated with Claude Code