Skip to content

dev-lead: switch on the maintainer no-change path safely (unrecorded-reply gap, live proof of stamping, cutoff wiring) — follow-up to #2079 / PR #2084 #2161

Closed
#2239
Closed
dev-lead: switch on the maintainer no-change path safely (unrecorded-reply gap, live proof of stamping, cutoff wiring) — follow-up to #2079 / PR #2084#2161
#2239
Labels
bugBug reportsdev-leadFor dev-lead agent pickup

Description

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

Context

PR #2084 "dev-lead: resolve a bot review thread on a maintainer 'no change needed' disposition" (issue #2079) added the no-change path but shipped it switched off. nochange_epoch_cutoff in scripts/dev-lead-fix-reviews.sh reads DEV_LEAD_NOCHANGE_EPOCH, and no workflow sets that variable, so no thread is resolved on a maintainer's no-change verdict yet. The complaint in #1743 is therefore not fixed.

This issue is the code work needed before the path can be switched on. It was re-scoped on 2026-10-10 to four criteria dev-lead can do in one PR; the maintainer-only steps are listed at the end and are not for dev-lead.

Already shown (was criterion 2): the recorder reaches the model. A ring0 pass stamped a real reply on PR #2135; see the 2026-10-10 comment below.

Acceptance criteria (dev-lead)

  1. Unstamped replies survive between passes. Today stamp_pass_replies writes ids it could not stamp to ${RUNNER_TEMP:-/tmp}/dev-lead-unstamped-replies-<pr>. Each pass runs on a fresh hosted runner, so the next pass never reads that file.
    • Keep the pending ids somewhere a later pass can read, for example a marker comment on the PR like dev-lead's other state markers.
    • The no-change path stays disabled on that PR until the pending list is empty.
    • Test: a pass that fails to stamp reply X, followed by a second pass with a fresh temp directory, still knows about X.
  2. An unattributable reply does not discard the attributable ones, and is not forgotten.
    • When the record holds UNATTRIBUTED, cl_recorded_reply_ids returns 1 and stamp_pass_replies sets recorded="", so replies whose ids were captured in that pass are neither stamped nor carried over. Stamp every id that was recorded, even when the pass as a whole is unattributable.
    • A reply whose id is unknown can never be stamped later. Persist that fact the same way as criterion 1, so the no-change path stays off for that PR until a human clears it.
    • The recorder currently flags any argument that starts with @ or contains =@ whose remainder is not a readable file (for example a comment body beginning with @coderabbitai). Narrow that so an ordinary comment is not treated as an unattributable reply. Keep the fail-closed behaviour for a real reply-creating call whose payload cannot be read.
    • Tests for each bullet. Commit 6adb065a (in PR feat: implement issue #2079 — dev-lead: resolve a bot review thread on a maintainer "no change needed" disposition (rebuild of #1799 on #2049's resolver) #2084) added none for the recorder's widened matching; add them.
  3. Resolver gaps CodeRabbit raised on PR feat: implement issue #2079 — dev-lead: resolve a bot review thread on a maintainer "no change needed" disposition (rebuild of #1799 on #2049's resolver) #2084.
    • In resolve_deferred_bot_threads the no-change branch runs after deferral parsing and the post-marker gate, so a thread with no deferral never reaches it. Either move it ahead of those gates, or remove the unreachable branch and document that resolve_addressed_bot_threads is the only entry point.
    • A failed resolveReviewThread on the no-change path in resolve_addressed_bot_threads only logs a warning. Make it retryable the way the deferred resolver is.
  4. Wire the cutoff, still off by default. Pass DEV_LEAD_NOCHANGE_EPOCH from an Actions variable into the dev-lead job in dev-lead-reusable.yml. Unset or unparseable must keep the path disabled, exactly as today. Do not set the variable.

Not for dev-lead (maintainer, after this merges)

Not in scope

Changing which phrases count as a no-change verdict.

Activity

  1. don-petry
    added
    bugBug reports
    on Oct 9, 2026

  2. github-actions commented on Oct 9, 2026

    @github-actions
    github-actionsBot
    on Oct 9, 2026 – with GitHub Actions
    Contributor

    Thanks for the detailed follow-up and the clear ordered checklist. The unrecorded-reply gap in item 1 is the blocker for switching the path on, so one thing would help before work starts: can you confirm the exact gh invocation forms (REST replies, in_reply_to, and GraphQL via --input or -F query=@file) you've seen the model use in real passes, so we can cover those in the shim and its tests?

  3. don-petry commented on Oct 10, 2026

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

    Evidence for acceptance criterion 2 (the recorder reaches the model), 2026-10-10.

    dev-lead/v139-ring0 moved to v139.52.0 (71920a24), which contains PR #2084. On PR #2135, a dev-lead fix-reviews pass on that release (run 38029736552) posted a review-thread reply that now carries <!-- dev-lead:reply -->: #2135 (comment)

    Of the 5 replies dev-lead posted on that PR since 03:00Z, 1 was stamped by the harness, 4 already carried another dev-lead marker, and none is marker-less. That is one PR and one pass, so it shows the recorder works on the instructed reply path; it does not cover the other call paths listed in criterion 1.

    Generated by Claude Code

  4. don-petry
    added
    dev-leadFor dev-lead agent pickup
    on Oct 10, 2026

  5. don-petry commented on Oct 11, 2026

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

    Dev-Lead Implementation Plan

    Issue: #2161 — dev-lead: switch on the maintainer no-change path safely (unrecorded-reply gap, live proof of stamping, cutoff wiring)

    Scope

    Make unstamped and unattributable pass replies durable across passes, narrow the recorder's @ matching, close the two resolver gaps CodeRabbit raised on #2084, and wire DEV_LEAD_NOCHANGE_EPOCH from an Actions variable. The path stays off by default.

    Implementation Checklist

    • scripts/lib/claim-landing.sh
      • Recorder shim: only gh api calls can be reply-creating. Read payload files only from --input and from -F/--field key=@file. A -f body=@coderabbitai … or gh pr comment --body "@coderabbitai …" is no longer flagged. An unreadable --input / -F @file on a reply call still fails closed.
      • cl_recorded_reply_ids: emit every captured id even when the record holds UNATTRIBUTED, and keep returning 1 in that case. New cl_record_unattributed predicate.
      • Pure helpers for a PR state marker: cl_pending_marker_body <ids> and cl_pending_marker_ids <body>. Marker: <!-- dev-lead:unstamped-replies ids=… -->.
    • scripts/dev-lead-fix-reviews.sh
      • stamp_pass_replies: drop the $RUNNER_TEMP file. Read the pending list from our own marker comment(s) on the PR (union), stamp recorded and pending ids, and carry an UNATTRIBUTED entry forward until a human clears it. Rewrite the marker, or delete it when the list is empty. A non-empty list, or a marker that cannot be read, keeps the no-change path off. A reply that no longer exists, or a node that is not a review comment, drops out of the list.
      • build_and_run: if no record file exists when the engine runs, the pass is marked unattributable.
      • resolve_deferred_bot_threads: move the no-change branch ahead of the own-reply/deferral parse and the post-marker gate. Bot-origin and full-page checks still run first.
      • resolve_addressed_bot_threads: a failed resolveReviewThread on the no-change path, or a response that is not isResolved: true, now counts as a failure, and the function returns non-zero. All three callers (fix-reviews, fix-bot-comment, review-changes) then post the resolve-failed retry marker and withhold auto-merge, as they already do for the deferred resolver.
    • .github/workflows/dev-lead-reusable.yml: DEV_LEAD_NOCHANGE_EPOCH: ${{ vars.DEV_LEAD_NOCHANGE_EPOCH || '' }} in the dev-lead job env. Unset or unparseable keeps the path disabled. The variable is not set.

    Tests to Write

    • Recorder: -f body=@coderabbitai … (raw field) is not flagged; gh pr comment --body "@coderabbitai …" is not flagged; -F body=@<missing> on a reply call fails closed; -F body=@file whose file holds a reply mutation is recorded; --input with an unreadable path fails closed.
    • cl_recorded_reply_ids emits the captured ids alongside UNATTRIBUTED (rc 1); cl_record_unattributed.
    • Marker body/parse round-trip, rejecting malformed ids.
    • stamp_pass_replies: pass 1 fails to stamp X, then pass 2 with a fresh temp dir still knows X from the PR marker and retries it; the marker is deleted once the list is empty.
    • stamp_pass_replies: a record holding UNATTRIBUTED plus Y stamps Y and persists UNATTRIBUTED; a later clean pass still returns non-zero.
    • resolve_deferred_bot_threads: a maintainer no-change verdict on a bot thread with no deferral resolves it (epoch set) on a no-commit pass, and does not resolve it with the epoch unset.
    • resolve_addressed_bot_threads: a failed no-change resolve returns non-zero and posts a resolve-failed retry marker.
    • Workflow: DEV_LEAD_NOCHANGE_EPOCH is mapped from vars in the dev-lead job.

    Test Command

    bats tests/dev-lead/unit/test_claim_landing.bats tests/dev-lead/unit/test_fix_reviews.bats (plus bash .dev-lead/scripts/dev-lead-lint.sh)

  6. coderabbitai commented on Oct 11, 2026

    @coderabbitai
    coderabbitaiBot
    on Oct 11, 2026 – with coderabbitai
    🧩 Analysis chain

    🏁 Script executed:

    #!/bin/bash
set -euo pipefail
fd --hidden --exclude .git '^(claim-landing\.sh|dev-lead-fix-reviews\.sh|dev-lead-reusable\.yml|test_claim_landing\.bats|test_fix_reviews\.bats|dev-lead-lint\.sh)$'
ast-grep outline scripts/lib/claim-landing.sh --items all || true
ast-grep outline scripts/dev-lead-fix-reviews.sh --items all --match 'stamp_pass_replies|build_and_run|resolve_deferred_bot_threads|resolve_addressed_bot_threads' || true
rg -n -C 3 'cl_recorded_reply_ids|UNATTRIBUTED|--input|raw-field|stamp_pass_replies|resolve_deferred_bot_threads|resolve_addressed_bot_threads|DEV_LEAD_NOCHANGE_EPOCH|resolve-failed' scripts/lib/claim-landing.sh scripts/dev-lead-fix-reviews.sh .github/workflows/dev-lead-reusable.yml tests/dev-lead/unit/test_claim_landing.bats tests/dev-lead/unit/test_fix_reviews.bats

    Length of output: 76848

    🏁 Script executed:

    #!/bin/bash
set -euo pipefail
sed -n '130,225p' scripts/lib/claim-landing.sh
sed -n '315,345p;1030,1095p;1410,1555p;3188,3243p;3690,3778p;3844,3885p;3974,4032p' scripts/dev-lead-fix-reviews.sh
rg -n -C 5 '^[[:space:]]*(dev-lead:|env:)|DEV_LEAD_' .github/workflows/dev-lead-reusable.yml | head -150
sed -n '465,530p' tests/dev-lead/unit/test_claim_landing.bats
sed -n '6918,6945p;6998,7036p' tests/dev-lead/unit/test_fix_reviews.bats
ast-grep outline scripts/dev-lead-lint.sh --items all || true
sed -n '1,85p' scripts/dev-lead-lint.sh

    Length of output: 40176

    🏁 Script executed:

    #!/bin/bash
set -eu
sed -n '465,530p' tests/dev-lead/unit/test_claim_landing.bats
sed -n '6918,6945p;6998,7036p' tests/dev-lead/unit/test_fix_reviews.bats
ast-grep outline scripts/dev-lead-lint.sh --items all || true
sed -n '1,85p' scripts/dev-lead-lint.sh

    Length of output: 9225

    @don-petry, thanks for the guidance. The plan covers the gaps in the current code. Keep DEV_LEAD_NOCHANGE_EPOCH unset.

    Add these safeguards to the implementation and tests:

    • Preserve pending state on API uncertainty. Drop a pending id only when the response confirms that the reply no longer exists or is not a PullRequestReviewComment. Keep the id on transport errors, GraphQL errors, or incomplete responses.
    • Validate mutation results. Both no-change resolver branches must require isResolved: true. HTTP success alone is not sufficient. The deferred branch currently also discards the response.
    • Handle addressed-resolver failures before terminal posting. All three callers currently use only || rc=1. On failure, set _REVIEW_INCOMPLETE=1, clear _AM_NEEDS_RESTORE, and post the retry marker. Do not post a success terminal first. Preserve the PR-wide fix-reviews retry intent for fix-bot-comment.
    • Test the complete recorder matrix. Include REST replies, REST in_reply_to, GraphQL through --input and -F query=@file, and stdin input. Assert that the shim preserves arguments, stdin, output, and exit status. These tests do not establish which forms the model used in live passes.
    • Fail closed on marker operations. Test pagination, multiple owned markers, malformed marker bodies, and failed marker writes or deletes.

    For a checkout of this repository, the lint command is bash scripts/dev-lead-lint.sh. The .dev-lead/scripts/dev-lead-lint.sh path is the workflow runtime path.

    • Update the recorder and pure pending-marker helpers.
    • Replace runner-local pending state with durable PR marker state.
    • Update both resolver branches and all three callers.
    • Wire the workflow variable without setting it.
    • Add the listed regression tests and the safeguards above.

    rather than runner-local files. UNATTRIBUTED persists until human clearance. Wire DEV_LEAD_NOCHANGE_EPOCH from Actions vars in .github/workflows/dev-lead-reusable.yml, but do not set the variable; the maintainer no-change path must remain disabled by default.
    -->

    ✏️ Learnings added 
    Learnt from: don-petry
URL: https://github.com/petry-projects/.github-private/issues/2161

Timestamp: 2026-10-11T00:18:28.138Z
Learning: For dev-lead issue #2161 in petry-projects/.github-private, the maintainer requires scripts/dev-lead-fix-reviews.sh to persist unstamped reply ids and UNATTRIBUTED state in owned PR comments using <!-- dev-lead:unstamped-replies ids=…

    Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

    💡 For code changes across multiple files, use @coderabbitai plan in an issue comment

    You are interacting with an AI system.

  7. don-petry commented on Oct 11, 2026

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

    Dev-Lead: Implementation Complete — PR #2239

    The implementation for issue #2161 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.

  8. don-petry
    added 3 commits that reference this issue on Oct 11, 2026

    feat: implement issue #2161 — dev-lead: switch on the maintainer no-c…

    b4ca6ad

    feat: implement issue #2161 — dev-lead: switch on the maintainer no-c…

    4f6070a

    fix(reviews): reject a no-change verdict when a bot finding was edite…

    124d811
  9. don-petry
    closed this as completedin #2239on Oct 11, 2026
  10. don-petry
    added a commit that references this issue on Oct 11, 2026

    Merge origin/main into fix/issue-2261-bot-issue-comment-disposition

    a1a3bb5
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