Skip to content

dev-lead: no deferred/out-of-scope outcome for review threads — a valid finding dev-lead defers stays unresolved forever and blocks merge #2045

Closed
#2049
Closed
dev-lead: no deferred/out-of-scope outcome for review threads — a valid finding dev-lead defers stays unresolved forever and blocks merge#2045
#2049
Labels
bugBug reportsdev-leadFor dev-lead agent pickup

Description

@don-petry
don-petry
opened on Oct 3, 2026

Summary

When dev-lead decides a bot review-thread finding is valid but out of scope for the PR, it replies with a skip note ("Valid, but deferring — out of scope") and the thread stays unresolved forever. Under required_review_thread_resolution that blocks merge, and no automated actor can clear it.

Evidence — PR #1953

Five Codex threads on scripts/canary_report.sh (lines 215, 362, 446, 466, 619) each got a dev-lead reply on 2026-10-01 such as:

Valid, but deferring — out of scope for this PR. … requires a per-case identifier in the telemetry …

None was resolved, and no tracking issue was opened or referenced for the deferred work. A maintainer resolved them by hand on 2026-10-03 to unblock the PR. The deferred findings themselves are still untracked:

  • emit_token_record writes no duration_ms, so the latency bar has no real data.
  • Fixed-judge calls are not excluded from controlled arms. This needs a generator/judge role field in telemetry.
  • Per-file parse/filter failures in the shared _collect_one_artifact are not propagated.
  • Controlled comparisons do not require paired cases. This needs a per-case id in telemetry.
  • Fallback-estimated cache usage is not treated as insufficient. This needs a measured/estimated provenance flag.

Root cause

The design is internally consistent, but it has no outcome for a deferred finding on a review thread:

  • prompts/dev-lead/fix-reviews.md allows the <!-- dev-lead:addressed --> marker plus a verified <!-- dev-lead:claim … --> only on a genuine fix. A skip note must carry no marker.
  • scripts/dev-lead-fix-reviews.sh (resolve_addressed_bot_threads) resolves only:
    • bot threads with a verified addressed claim;
    • outdated bot threads.
  • Agents are forbidden from calling resolveReviewThread.

PR issue comments already have the missing outcome. disposition=out-of-scope ref=#<n> requires the tracking issue to exist, and once verified the harness clears the comment (#1813, AC6: one tracking issue per repo). Review threads have no equivalent.

#1743 / PR #1799 does not cover this. It handles false positives, and only after a marker-less human maintainer posts a "no change needed" reply. A deferral is a real finding that should be tracked, not dismissed.

Proposed fix

Add a deferred outcome for bot-originated review threads that mirrors the issue-comment out-of-scope disposition.

  1. Prompt. On a bot thread whose finding is valid but out of scope, dev-lead replies with a deferral marker, e.g. <!-- dev-lead:deferred ref=#<n> -->, where #<n> is the repo's single deferred-findings tracking issue. dev-lead opens or reuses that issue and appends the finding with a link to the thread.

  2. Harness. Resolve a bot thread when all of these hold:

    • its latest our-account reply carries exactly one deferral marker;
    • ref names an issue that exists and is open;
    • that issue's body or comments link the thread or the originating comment.

    It runs on no-commit passes too, because a deferral produces no commit.

  3. Guards.

Acceptance criteria

Generated by Claude Code

Activity

  1. don-petry
    added
    bugBug reports
    dev-leadFor dev-lead agent pickup
    on Oct 3, 2026

  2. don-petry commented on Oct 3, 2026

    @don-petry
    don-petry
    on Oct 3, 2026
    CollaboratorAuthor

    Dev-Lead Implementation Plan

    Issue: #2045 — dev-lead: no deferred/out-of-scope outcome for review threads

    Scope

    Add a verified deferred outcome for bot-originated review threads (<!-- dev-lead:deferred ref=#<n> -->), resolved by the harness on commit and no-commit passes once the referenced tracking issue exists, is open, and links the thread.

    Implementation Checklist

    • scripts/lib/deferred-thread-verify.sh (new, pure dtv_* helpers): parse exactly one deferral marker → ref; find the latest our-account reply; decide whether a tracking issue (open, not a PR) mentions the thread (thread node id, originating comment URL, or discussion_r<databaseId>).
    • scripts/dev-lead-fix-reviews.sh: new resolve_deferred_bot_threads, which:
      • re-reads each unresolved bot thread;
      • requires the latest our-account reply to carry exactly one deferral marker;
      • requires nothing unaddressed after it (acv_post_marker_clear);
      • withholds on any maintainer "required" disposition (acv_latest_maintainer_disposition);
      • verifies the ref'd issue via gh api;
      • fails closed at each step.
        It is wired next to resolve_dispositioned_comments (outside the Follow-up to #1609 (slice 2): gate dev-lead thread resolution on the pass having advanced the PR head #1617 head-advance gate) in fix-reviews, fix-bot-comment and review-changes. The bot-thread enumeration is shared with resolve_addressed_bot_threads.
    • prompts/dev-lead/fix-reviews.md: document the deferral reply. It reuses the single per-repo tracking issue (AC6, shared with out-of-scope) and appends the finding with the thread link.
    • .github/workflows/lint.yml: add the new bats file to the repo-specific list.
    • Open a tracking issue for the five feat: implement issue #1951 — [Phase 2] Canary go/no-go report: compare deep-tier cost, cache-read cost and latency by model from token-usage artifacts #1953 deferrals.

    Tests to Write

    • tests/dev-lead/unit/test_deferred_thread_verify.bats: the pure parser, the mention matcher and the issue-state checks. Cases: missing ref, bad ref, more than one marker, closed issue, PR instead of issue, no mention.
    • tests/dev-lead/unit/test_fix_reviews.bats: harness cases.
      • Resolves on a no-commit pass and on a commit pass.
      • Leaves the thread open for each of: nonexistent ref, closed issue, issue that does not mention the thread, two markers, maintainer thread, maintainer "required" disposition.

    Test Command

    bats tests/dev-lead/unit/ (+ bash .dev-lead/scripts/dev-lead-lint.sh)

  3. don-petry commented on Oct 3, 2026

    @don-petry
    don-petry
    on Oct 3, 2026
    CollaboratorAuthor

    Dev-Lead: Progress (in progress — not yet durable)

    Provisional. Nothing is pushed yet. The automation posts the authoritative completion record (PR number + head SHA) after the push. Treat the notes below as intent, not delivered results.

    Plan execution (this workspace)

    Notes

    • Stricter than the issue text: a deferral is withheld when a maintainer "required" disposition appears anywhere in the thread, not only a newer one. A deferral fixes nothing, so it should never overrule a maintainer who said the fix is required.
    • A thread counts as "linked" when the tracking issue (body or comments) contains the thread node id (PRRT_…) or the originating comment's discussion_r<id> anchor.

  4. don-petry commented on Oct 3, 2026

    @don-petry
    don-petry
    on Oct 3, 2026
    CollaboratorAuthor

    Dev-Lead: Implementation Complete — PR #2049

    The implementation for issue #2045 is durable: commits are pushed and a pull request is open. This record is backed by verifiable artifacts you can confirm in one click.

    Review happens on the PR. Acceptance criteria and test results are evidenced by the PR's diff and CI checks — not asserted here. If a later stage fails, this claim is retracted in place.

  5. don-petry
    added 17 commits that reference this issue on Oct 3, 2026

    feat: implement issue #2045 — dev-lead: no deferred/out-of-scope outc…

    77b46e7

    fix: handle command substitution failures under set -euo pipefail in …

    7927246

    feat: implement issue #2045 — dev-lead: no deferred/out-of-scope outc…

    9bd30cd

    fix: handle command substitution failures under set -euo pipefail in …

    ab4c109

    fix(reviews): fail closed on tracker-comment pagination errors, split…

    a59308f

    feat: implement issue #2045 — dev-lead: no deferred/out-of-scope outc…

    70f0084

    fix: handle command substitution failures under set -euo pipefail in …

    f153197

    fix(reviews): fail closed on tracker-comment pagination errors, split…

    1ebb0be

    fix(reviews): make deferred-thread resolution work and recover on fai…

    b670c10

    fix(reviews): count an unreadable edit time as a failure; require a c…

    3a93825

    fix(reviews): record resolve-failed as a non-quota hold; a malformed …

    bb38d3b

    fix(reviews): reason-aware hold dedup and an accurate resolve-failed …

    38adcb1

    fix(dev-lead): fail closed on partial deferral snapshots; retry fix-b…

    edb92ae

    fix(dev-lead): key resolve-failed retry markers to the pushed head (#…

    22cb533

    fix(dev-lead): retry deferral resolution failures on failed passes too (

    642ebc0

    fix(dev-lead): stop on a non-advancing thread cursor; never retitle h…

    7bf83ec

    feat: implement issue #2045 — dev-lead: no deferred/out-of-scope outc…

    df4bb51
  6. don-petry
    closed this as completedin #2049on Oct 9, 2026
  7. don-petry
    added 4 commits that reference this issue on Oct 9, 2026

    fix(dev-lead): trusted-author tracker links; no auto-merge after a fa…

    c248d4a

    fix(dev-lead): trusted-author tracker links; no auto-merge after a fa…

    6415eab

    fix(dev-lead): an issue-comment listing that is not a list no longer …

    95fb912

    fix(dev-lead): disposition a fixed bot issue comment, and sweep claim…

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugBug reportsdev-leadFor dev-lead agent pickup

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions