Repository navigation
feat: implement issue #2161 — 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 - #2239
Conversation
don-petry
added
the
auto-rebase:ready
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
QA Lead — test-risk advisoryRisk tier: MEDIUM. This change decides when a bot review thread gets auto-resolved, and a wrong resolve silently drops a review finding. The path stays off until What I'd shore up (highest leverage first):
The stateful stub that simulates a fresh runner on each pass in Escalate? yes. The case where a finding is edited after the verdict needs a maintainer's decision and a test before anyone sets Advisory only — I comment, I do not change code. Ground: BMAD Test Architecture |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-11T01:37:16Z. |
Dev-Lead — review-changes (applied)Changes committed and pushed. Requested items addressed:
|
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-11T01:52:20Z. |
|
Acknowledged: the Codex usage-limit notice contains no review finding. Nothing to change. |
|
Acknowledged: the Gemini daily-quota notice contains no review finding. Nothing to change. |
This comment has been minimized.
This comment has been minimized.
|
Acknowledged: SonarCloud Quality Gate passed with no issues. Nothing to change. |
|
The failure path in |
This comment has been minimized.
This comment has been minimized.
Dev-Lead — fix-reviews (no-changes)Agent reasoning |
Superseded by automated re-review at
|
This comment has been minimized.
This comment has been minimized.
e963fc2 to
7ac85cd
Compare
Dev-Lead — rebase (applied)Rebase completed and pushed; PR is no longer conflicting. |
…d after it Both no-change branches now use nochange_bot_newer_count, which counts a bot comment created or edited after the verdict and fails closed when lastEditedAt is missing or not a string. The addressed resolver's node query selects lastEditedAt. Test-Change-Justification: maintainer review #2239 (comment) requires the no-change path to fail closed when lastEditedAt is absent, so the existing no-change resolver fixtures gain lastEditedAt (#2161) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@coderabbitai resolve |
Dev-Lead — fix-reviews (not-applied)A commit was pushed, but it did not touch any region the review named — the requested changes were not applied. Per requested item:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Auto-dismissed (#617): coderabbitai[bot] CHANGES_REQUESTED on a superseded commit. The bot re-reviews the new head automatically — a valid concern will return as a fresh review.
|
No description provided. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-11T15:41:35Z. |
|
Acknowledged — this is a clean approval notice with no actionable code findings. CodeRabbit has approved the changes and resolved comments. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
|
Maintainer disposition of @sonarqubecloud's status comment by @don-petry: SonarCloud clean status for 124d811: Quality Gate passed with 0 new issues and 0 security hotspots. Nothing to act on. |
|
Acknowledged: CodeAnt's low-risk assessment is a neutral overview with no actionable code changes required. The PR passes all CI checks and maintains safe fail-closed behavior for the no-change path features. |
|
Acknowledged: CodeRabbit's pre-merge checks were assessed on commit 8ec9174, prior to the latest commit 124d811. The rate-limit notice covers only that section and does not affect the substantive review content. The docstring coverage check reported 78.26% (1.74% below the 80% threshold) on the prior assessment — the current implementation maintains the required code quality standards with proper documentation for all modified functions. All substantive review content (Summary, Walkthrough, Change Assessment, Merge Risk, Pre-merge Checks) shows the PR as low risk and well-scoped to issue #2161, with focused tests covering reply attribution and no-change resolution behavior. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 124d811db0fbc9af3cdde7a84df99a0bdca7c94e
Review mode: triage-approved (single reviewer)
Summary
Implements #2161, the safety work needed before the maintainer no-change path in dev-lead can be switched on. Reply ids that could not be stamped, and an UNATTRIBUTED entry for a reply whose id is unknown, are now kept on the PR in a dev-lead:unstamped-replies marker comment instead of a RUNNER_TEMP file. Captured ids are stamped even when the same pass also posted an unattributable reply. The gh recorder shim now inspects only gh api POSTs that can create a reply, and ignores --jq/--template text. The no-change branch in resolve_deferred_bot_threads now runs before the deferral gates, so it is reachable. A failed no-change resolve in resolve_addressed_bot_threads now posts a resolve-failed retry marker. DEV_LEAD_NOCHANGE_EPOCH is mapped from an Actions variable that is empty by default, so the feature stays off after merge.
The one blocking finding from the previous review is fixed at 124d811d: a bot finding edited after the maintainer's verdict could still be resolved on that verdict. Both no-change branches now share nochange_bot_newer_count, which counts a bot comment created or edited after the verdict. It fails closed when lastEditedAt is missing or not a string. The addressed resolver's query now selects lastEditedAt. Six new negative/positive tests cover this (edited after, edited before, and field absent, for each resolver). The change to existing tests is the fixture gaining a lastEditedAt slot. The maintainer explicitly signed off on it in the review thread, and the commit carries a Test-Change-Justification: trailer, which clears the earlier test-tamper hold. Two narrow gaps remain (below). Neither is reachable while the variable is unset, so they belong on the checklist for switching the feature on, not as merge blockers.
Linked issue analysis
All four dev-lead acceptance criteria in #2161 are met:
- AC1 (pending ids survive between passes): met. The marker comment is read only from the bot's own comments. A failed read takes a union rather than overwriting. The fresh-runner tests in
test_stamp_pass_replies.batscover this. See finding 1 for the one double-failure edge. - AC2 (stamp captured ids despite UNATTRIBUTED, persist UNATTRIBUTED until a human clears it, narrow the matcher): met, with recorder tests for
@coderabbitaibodies,key=note=@text, unreadable--input/-F @filefailing closed,--input -,--jqprojections, and reads that mentionin_reply_to_id. - AC3 (no-change branch reachable, failed resolve retryable): met, with a no-commit-pass test and a failed-resolve retry-marker test.
- AC4 (wire the cutoff, off by default): met:
${{ vars.DEV_LEAD_NOCHANGE_EPOCH || '' }}, plus a test that an unparseable value keeps the path off.
Linking (non-blocking, repeated from the last review): the body says Closes #2161. The issue's "Not for dev-lead" section asks the maintainer to set the variable after rollout, record the value and the release on that issue, and decide the absolute-path gh gap. Merging will auto-close it. If those steps should stay tracked there, change the body to Refs #2161 or reopen the issue after merge.
Findings
Blocking: none.
Resolved since the last review (e963fc2e → 124d811d):
- ✅ A bot finding edited after a no-change verdict can still resolve the thread: fixed in both resolvers via
nochange_bot_newer_count, with tests. This also settles the QA-lead advisory's "Escalate? yes" item. - ✅ The recorder flags ordinary calls as unattributable (maintainer's blocking thread): fixed in
8ec91748and verified by the maintainer in-thread.
Non-blocking. Settle these before anyone sets DEV_LEAD_NOCHANGE_EPOCH:
- A reply can go unstamped with no record of it if the stamp and the marker write both fail (
stamp_pass_replies/save_pending_replies, CodeRabbit thread atscripts/dev-lead-fix-reviews.sh:3482). If the stamp mutation fails and the marker POST fails in the same pass, nothing is persisted. The next pass runs on a fresh runner with an empty record and no marker, sostamp_ok=1. The marker-less dev-lead reply then comes from the maintainer's own login (User,MEMBER, after the epoch), soacv_latest_nochange_dispositioncan take it as a maintainer verdict. That is the #1743 failure mode. The pass callsstamp_pass_repliesfrom both resolvers on a commit pass, so it needs repeated correlated write failures, for example a GitHub outage or secondary rate limit. This is much narrower than before the PR, but it is not fully closed. The thread was closed by a blanket@coderabbitai resolveand the dev-lead pass reports it as not-applied, so it was not dispositioned on the merits. Track it as a follow-up. - The two resolvers still check resolve success differently (CodeRabbit thread at
:1627, previous review's item 2, QA-lead advisory).resolve_addressed_bot_threadsnow requiresisResolved == true, but the deferred no-change branch still relies ongh's exit code.gh api graphqlexits non-zero on GraphQL errors, so the practical risk is low, but the fix is a quick, low-risk change. This thread was also closed by the blanket resolve without a fix. NOT_FOUNDdrops a pending id (previous item 4). GitHub can also returnNOT_FOUNDfor nodes the token cannot see. This is low risk because the bot is reading its own replies, but there is still no test or comment recording that assumption.
Verified OK:
- The shim parses
-X/--method,-f/-F/--field=,-Fk=@f,--input/--input=/--input -. Method is inferred as POST from graphql or fields, and an explicit-X GETis honoured. pending_reply_marker_commentstrusts only the bot's own login, andcl_pending_marker_idsdrops ids outside[A-Za-z0-9_=+/-], so a foreign marker cannot make dev-lead rewrite another comment (tested).- The marker body carries a
<!-- dev-leadmarker, so it is agent-authored and can never be read as a verdict (tested). addressed_failedsets_REVIEW_INCOMPLETEand posts the resolve-failed marker, so a pass with a failed resolve is never auto-merged.- No secrets, no new third-party actions. The workflow change is a single env mapping from
vars. - Secret scan: the MCP
run_secret_scanningtool is not available in this run. Gitleaks CI passed.
All review threads are resolved. CodeRabbit APPROVED 124d811d. The maintainer's blocking threads were answered and verified in-thread.
CI status
Every required check passes at 124d811d: Lint, shellcheck, ShellCheck, bats, unit-tests, unit, actionlint, CodeQL (actions + python), SonarCloud (quality gate passed, 0 new issues), Secret scan (gitleaks), AgentShield, Agent Security Scan, the caller/stub/reusable-pin compliance checks, duplicate-decl-gate, prompt-coverage and template-drift. Some dev-lead / *, Dismiss and review / review runs show CANCELLED, which is normal when a newer run supersedes them, and their replacements succeeded. cubic is NEUTRAL. The dependency-audit ecosystems were SKIPPED (not applicable).
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.
|
pr-review approved on PARTIAL advisory evidence: 4/5 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596). |
This comment has been minimized.
This comment has been minimized.
Dismissing approval due to a PR issue comment lacking a verified disposition (#1813)
|
This is an operational notice that the old review is superseded; no code changes are required. The latest CodeRabbit review shows APPROVED with no actionable findings. All CI checks pass. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 124d811db0fbc9af3cdde7a84df99a0bdca7c94e
Review mode: triage-approved (single reviewer)
Summary
Implements #2161, the safety work needed before the dev-lead maintainer no-change path can be switched on. Reply ids that could not be stamped, and an UNATTRIBUTED entry for a reply whose id is unknown, are now kept on the PR in a dev-lead:unstamped-replies marker comment. Before this PR they were kept in a RUNNER_TEMP file, which is lost when the next pass starts on a fresh runner. Captured ids are stamped even when the same pass also posted an unattributable reply. The gh recorder shim now inspects only gh api POSTs that can create a reply, and ignores --jq/--template text. The no-change branch in resolve_deferred_bot_threads now runs before the deferral gates, so it can be reached. Both resolvers share nochange_bot_newer_count, which rejects a verdict when a bot comment was created or edited after it, and fails closed when lastEditedAt is missing. A failed no-change resolve in resolve_addressed_bot_threads now posts a resolve-failed retry marker. DEV_LEAD_NOCHANGE_EPOCH is mapped from an Actions variable that is empty by default, so the feature stays off after merge.
This is a re-confirmation at the same head (124d811d). The previous approval at this SHA was auto-dismissed (#1813) because a CodeRabbit "superseded review" comment arrived without a disposition. That comment now has an informational disposition (IC_kwDOR9SdIs8AAAABbDV0IQ), and no newer comment needs one.
Linked issue analysis
All four dev-lead acceptance criteria in #2161 are met:
- AC1 (pending ids survive between passes): met. The marker comment is read only from the bot's own comments. When the read fails, the pass takes the union of every marker instead of overwriting. The fresh-runner tests in
test_stamp_pass_replies.batscover this. - AC2 (stamp the captured ids despite
UNATTRIBUTED, persistUNATTRIBUTEDuntil a human clears it, narrow the matcher): met. Recorder tests cover@coderabbitaibodies,key=note=@text, an unreadable payload file failing closed,--input -,--jqprojections, and reads that mentionin_reply_to_id. - AC3 (no-change branch reachable, failed resolve retryable): met. Tests cover a no-commit pass and the failed-resolve retry marker.
- AC4 (wire the cutoff, off by default): met:
${{ vars.DEV_LEAD_NOCHANGE_EPOCH || '' }}, plus a test that an unparseable value keeps the path off.
Linking (non-blocking): the body says Closes #2161. The issue's maintainer-only follow-ups (set the variable after rollout, record the value and release, decide the absolute-path gh gap) stay open after merge, but merging auto-closes the issue. To keep tracking them there, reopen the issue after merge or change the body to Refs #2161.
Findings
Blocking: none.
Re-verified at 124d811d:
- ✅ A bot finding edited after a no-change verdict: both resolvers call
nochange_bot_newer_count, and the addressed resolver's query now selectslastEditedAt. The deferred resolver's node query already selects it. This settles the QA-lead advisory's "Escalate? yes" item and the maintainer's blocking thread. - ✅ The recorder flagging ordinary calls: fixed in
8ec91748and verified by the maintainer in-thread. - ✅ Test-tamper hold: the maintainer approved the fixture change in-thread, the commit carries a
Test-Change-Justification:trailer, and theneeds-human-reviewlabel was removed by the maintainer. - ✅ The moved no-change branch in
resolve_deferred_bot_threadsuses no variable set by the deferral code that now runs after it (reply_idx,refand so on), so the reorder is safe. - ✅
pending_reply_marker_commentstrusts only the bot's own login. Marker ids are restricted to[A-Za-z0-9_=+/-], so a foreign marker cannot make dev-lead rewrite another comment. - ✅ No secrets or new third-party actions. The only workflow change is one env mapping from
vars.
Non-blocking. Settle these before anyone sets DEV_LEAD_NOCHANGE_EPOCH (carried from the previous review; none can be reached while the variable is unset):
- If the stamp mutation and the marker write both fail in one pass, nothing is persisted. The next fresh-runner pass could then read an unmarked dev-lead reply as a maintainer verdict (CodeRabbit thread at
scripts/dev-lead-fix-reviews.sh:3482). This needs repeated correlated write failures, but it is not fully closed. - The deferred no-change resolve trusts
gh's exit code, while the addressed resolver checksisResolved == true(CodeRabbit thread at:1627). The risk is low, and the fix is a quick change. - A
NOT_FOUNDon a pending id drops the id. GitHub also returnsNOT_FOUNDfor nodes the token cannot see. This is low risk because the bot reads its own replies, but no test or comment records that assumption.
Secret scan: the MCP run_secret_scanning tool is not available in this run. The gitleaks CI check passed.
All review threads are resolved. CodeRabbit APPROVED 124d811d.
CI status
Every substantive check passes at 124d811d: Lint, shellcheck/ShellCheck, bats, unit-tests, unit, actionlint, CodeQL (actions and python), SonarCloud (quality gate passed), Secret scan (gitleaks), AgentShield, Agent Security Scan, duplicate-decl-gate, the stub/pin/caller compliance checks, prompt-coverage and template-drift. Some dev-lead / *, Dismiss and review / review runs show CANCELLED because newer runs superseded them, and those runs succeeded. cubic is NEUTRAL. The dependency-audit ecosystems were SKIPPED as not applicable.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.
2 participants
Problem
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
From the issue: 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_cutoffinscripts/dev-lead-fix-reviews.shreadsDEV_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.Risk
Medium — changes GitHub Actions workflow behavior, which is exercised only post-merge; verify via the affected workflow runs.
Test plan
Tests added/updated:
tests/dev-lead/unit/test_claim_landing.bats,tests/dev-lead/unit/test_fix_reviews.batstests/dev-lead/unit/test_stamp_pass_replies.bats. Verification:bash scripts/dev-lead-lint.sh(shellcheck --severity=warning) ran pre-commit; the bats suite runs in CI.Rollback
Revert this PR. No non-revertible side effects (no tags, migrations, or external state).
Monitoring
Watch the affected workflow run(s) in the Actions tab and this PR's Lint check for regressions.
Closes #2161