fix(backlog): transition item to pr_pending when PR created via RunOneShot#160
Merged
Merged
Conversation
…d via RunOneShot The Review Queue's manual "Create PR" button drives SessionService.RunOneShot, which creates a real PR but only ever persisted the PR URL onto the session record — it had no knowledge of backlog items at all. pushAndCreatePR (the automated review-PASS path) is the only other place that writes pr_pending, so any backlog item whose PR was created via the manual flow was silently left in "review" forever, invisible to ReconcilePRPending's FindPRPendingItems query. This is the root cause behind PR #157's linked backlog item being stuck at status review/BOUNCING instead of pr_pending. Add BacklogLifecycleListener.NotifyPRCreatedOutOfBand, called from RunOneShot after a PR URL is extracted, which stores the PR fields and transitions review -> pr_pending using the same precondition-guarded sequence as pushAndCreatePR. No-op when the listener is disabled, the session isn't backlog-linked, or the item isn't currently in review. 4 regression tests cover the transition, the non-backlog-session no-op, the non-review-status no-op, and the disabled-listener no-op. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
✅ Registry ValidationTest Coverage: 8/172 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
…OfBand Four-agent parallel code review (testing, code quality, architecture, security) on the prior commit surfaced: - Testing: the resolveStuckLogged side effect (clearing a stale abandoned_review row) was never actually exercised by any of the 4 new tests. Added TestRecordPRCreatedOutOfBand_ClearsAbandonedReviewStuckReason_ WhenItemWasStuck, which marks an item stuck first and asserts the row is gone afterward. - Code quality: extracted the identical transition+resolve-stuck tail shared by pushAndCreatePR and the new method into resolveToPRPending, removing the duplication. Also renamed NotifyPRCreatedOutOfBand -> RecordPRCreatedOutOfBand — "Notify" collided in spirit with the existing l.notify() toast helper in this file, but this method mutates DB state, it doesn't just surface a message. - Code quality (concurrency): the doc comment overstated TransitionBacklogItemStatus's guarantee as "exactly one of two racing calls wins" — verified against session/ent_repository_backlog.go and it's actually a read-then-write with no WHERE-clause CAS, so both could succeed under a true race (harmless here since both write the same target state, but the comment was wrong). Corrected the wording. - Security: added workSessionUUID to the success log line so an operator can trace which session caused a given transition. Not fixed here (documented in the method's doc comment as known limitations, consistent with this codebase's existing pattern of tracking follow-ups rather than scope-creeping a bug fix): this path doesn't call EnablePRAutoMerge like pushAndCreatePR does (no worktree handle available), and prURL/prNumber come from unverified freeform LLM stdout parsing (extractPRURL) rather than a GitHub-verified check — acceptable for this single-operator tool's threat model today. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
🎬 E2E Feature Demos2 shard(s) recorded feature flows for this PR. recordings shard 1 Demo preview opens directly in browser (single-file HTML). Raw WebM recordings in ZIP. Expires after 30 days. |
Contributor
✅ Registry ValidationTest Coverage: 8/172 features have
|
Contributor
Frontend Terminal Throughput |
…log-pr-status-desync-runoneshot
Contributor
✅ Registry ValidationTest Coverage: 8/172 features have
|
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the status-desync bug behind PR #157's linked backlog item being stuck at
review/BOUNCINGinstead ofpr_pending— a backlog item whose PR was createdthrough the Review Queue's manual "Create PR" button never transitioned status,
making it invisible to
ReconcilePRPending's conflict-detection reconciler.(PR #157 itself was independently closed on 2026-07-17 — its fix had already
landed on
mainvia unrelated commits — verified viagh pr view 157anddiffing
mainagainst the commits cited in that closing comment.)Context
docs/tasks/backlog-feature-improvement.mdtraced this exact desync but leftthe root cause unidentified: "Not yet root-caused to a specific line; next
step is tracing every write path that sets a backlog item's status to
pr_pending in
backlog_service_lifecycle.go."Root cause:
pushAndCreatePR(session/backlog_lifecycle.go) is the onlycode path that ever writes
pr_pending, and it's reached exclusively via theautomated
handleReviewSessionExited(PASS)chain. The Review Queue's manual"Create PR" button (
ReviewQueuePanel.tsx) instead callsSessionService.RunOneShot, which runs an ad hocclaude -p <prompt>in theworktree and only ever persists the resulting PR URL onto the session
record (
inst.SetGitHubPR) — it has zero knowledge of backlog items. Abacklog-linked item whose PR was created this way stayed in
reviewforever,accumulating unrelated
in_progress<->reviewbounce churn and eventuallyreporting
BOUNCINGinstead of the correctpr_ready_unmerged.Changes
session/backlog_lifecycle.go: newBacklogLifecycleListener.NotifyPRCreatedOutOfBand— stores the PR URL/number on the backlog item and transitions
review->pr_pending, mirroringpushAndCreatePR's own precondition-guardedsequence. No-op if the listener is disabled, the session isn't backlog-linked,
or the item isn't currently
review(so it never fightspushAndCreatePRina race).
server/services/session_service.go:RunOneShotnow callsNotifyPRCreatedOutOfBandafter extracting a PR URL from the CLI output.docs/registry/features/backend/session/run-one-shot.json: bumpedlastModifiedfor the handler change.session/backlog_lifecycle_test.gocovering the transition, the non-backlog-session no-op, the
non-review-status no-op, and the disabled-listener no-op.
Impact
session/backlog_lifecycle.go,server/services/session_service.go.path when a PR URL was extracted.
Testing
go test ./session -count=1— passgo test ./server/services -count=1— passgo vet ./session/... ./server/services/...— cleangolangci-lint run ./session/... ./server/services/...— 0 issuesgofmt -lon changed files — cleanReviewer Notes
ExpectedStatus: review) is whatkeeps this safe under a race with
pushAndCreatePR— worth double-checkingthat reasoning.
RunOneShotitself still has no direct RPC-level Gotest (pre-existing gap,
docs/registry/features/backend/session/run-one-shot.jsonstill shows
tested: false) — the new coverage is at theNotifyPRCreatedOutOfBandunit level, matching howpushAndCreatePR's ownregression tests are structured in this file.
docs/tasks/backlog-feature-improvement.md's audit trail forthis bug lives in an unpushed local commit on a separate checkout's
mainbranch (not part of this PR's history) — whoever pushes that commit should
cross-reference this PR.