fix(productivity-review): suppress dependency-blocked reviews

[kkroo]

Summary

Independent kkroo-authored successor to #1158. The original remains untouched because its app/allyblockcast author cannot supply the required independent App review.

• Exclude issue_dependencies_blocked cancellations from productivity streaks while retaining useful non-executing telemetry.
• Batch dependency-readiness checks in reconciliation, with a dedicated dependencyBlockedSkipped counter.
• Retire already-open productivity reviews when their source becomes blocked, under the same per-source advisory lock used by dependency-relation writers.
• Keep continuation-hold evaluation separate from reconciliation so an existing soft-stop hold remains effective until the review is retired.
• Avoid presenting a missing error code as literal unknown, and omit a tied diagnostic rather than choosing by insertion order.

Review feedback addressed

Addresses both Important findings and the actionable Suggestions from the original exact-head review: stranded open reviews, continuation-hold scope, measurable skips, transparent infra-failure streaks, and unambiguous non-executing diagnostics.

Validation

• pnpm exec vitest run server/src/__tests__/productivity-review-service.test.ts -t "BLO-22436|distinguishes a missing error code|non-executing error-code diagnosis" — 8 passed
• pnpm --filter @paperclipai/server typecheck — passed
• Full productivity-review suite exercised 105 assertions successfully; one run hit its existing embedded-Postgres cleanup-hook timeout after assertions completed.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[allyblockcast]

🔗 Paperclip issue: BLO-22436

[allyblockcast]

🔗 Paperclip issue: BLO-22436

[allyblockcast]

Hey @kkroo! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[kkroo]

@ally please review exact head eaafa3a8ee9f3f9c0a36540b1da6e6be31300fd0.

This is the independent kkroo successor to #1158; the App-authored original remains untouched. Please post the canonical App review for this SHA only, with the standalone Reviewed head: eaafa3a8ee9f3f9c0a36540b1da6e6be31300fd0 line and ally-verdict: pass if clean.

Review focus:

1. reconciliation-only dependency gate preserves continuation holds;
2. open review retirement holds the same per-source blocker lock as relation writers;
3. dependency cancellations stay transparent to both productivity streaks and telemetry does not invent a dominant error code on ties.

Validation on this head: 8 focused cases passed; server typecheck passed. Auto-merge is intentionally off.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: eaafa3a

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/services/productivity-review.ts:1947 — The dependency-blocked close transaction only takes the dependent/source advisory lock, then reads blocker status without a row lock or a closing EXISTS predicate. A concurrent update that marks the blocker done does not acquire this dependent lock (server/src/services/issues.ts:9164 only locks it when changing that issue's own blocker relations), so it can commit after lockedReadiness reports unresolved but before lockedClosed runs. The review is then closed and enters the six-hour resolved-review snooze even though the source is already runnable.
  • Revalidate the blocker state atomically with the review update, for example by locking the blocker rows used by readiness or adding a readiness-aware predicate to the close; add a concurrency test for a blocker transitioning to done during this close path.

Suggestions (0)

Strengths

• The dependency-gate run classification correctly keeps cancelled queued runs out of both productivity streaks while preserving explicit telemetry.
• The tests cover resolved historical cancellations, open-review suppression, continuation-hold behavior, and ambiguous error-code reporting.

Recommended Action

1. Fix the Important race before merge.
2. Re-run the focused productivity-review service suite.

[kkroo]

Closing as stale — this draft can no longer be landed as a diff.

Measured against master ac91e966:

• 822 commits behind
• 2 conflicting file(s) (git merge-tree --write-tree)

At that distance the surrounding code has moved out from under the patch, so "resolve the conflicts" would mean rewriting it against a codebase it was never written for — not merging it. Reopening is cheap if the branch is still wanted; the commits are untouched.

Closing the PR does not close the underlying issue. Where this PR referenced a BLO issue, that issue remains the record of intent and the place to re-implement from, against current master.

No judgement here on whether the original problem still exists — I did not verify that per-PR, and it should be re-checked before any re-implementation, since some of these may already have been fixed independently.