[don-petry]

Problem

pr-review reaches approve but cannot post it (approval marker missing), and the same head gets approve/LOW in one run and escalate/HIGH in the next

From the issue: A pr-review cascade can reach an approve verdict and then fail to post it, because the synthesized approval body does not contain a complete approval marker. The #1754 guard correctly refuses to submit, but the run then exits 1 with nothing written to the PR. A re-run redoes the whole cascade and can reach a different verdict, so the outcome for the same head SHA depends on which run happens to post.

Risk

Low — changes automation shell logic under scripts/, covered by shellcheck (--severity=warning) and the bats suite.

Test plan

Tests added/updated: tests/test_post_pr_review_escalation.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

This PR's Lint (shellcheck) and bats checks show pass/fail; watch subsequent dev-lead / pr-review runs for behavioral regressions.

Closes #2169

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[gemini-code-assist]

Code Review

This pull request addresses issue #2169 by ensuring that the approval marker decision word is normalized to "approved" (instead of "approve") to satisfy the formatting guard. It updates prompts/cascade-action.md to map the decision value before substitution, adds a normalization step in scripts/post-pr-review.sh using sed, and introduces comprehensive BATS tests to verify this behavior. Feedback on the changes highlights an issue in scripts/post-pr-review.sh where Bash command substitution strips trailing newlines, causing the comparison check to incorrectly evaluate to true and strip trailing newlines from the body even when no substitution occurred, and provides a suggestion to preserve them.

[don-petry]

Dev-Lead — review-changes (partial)

A commit was pushed, but not every requested change was applied. Per requested item:

• scripts/post-pr-review.sh:144 — not applied
• scripts/post-pr-review.sh:140 — applied

The unaddressed items above still need work.

[don-petry]

QA Lead — test-risk advisory

Risk tier: MEDIUM — this is the auto-approval gate. The fix is narrow and fails closed, but nothing tests the contract between the script that writes the marker and the scripts that read it.

What I'd shore up (highest leverage first):

• Contract test across the boundary. The test decision=approve directly before the marker close posts sha=X risk=LOW decision=approved-->. That passes the pr-review counts a human-escalation as a posted review — run reports "1 reviews posted" with nothing on the PR #1754 check in post-pr-review.sh, but review-one-pr.sh:762, carry-forward.sh:45 and sweep-stuck-reviews.sh:376 only match sha=<SHA>\s+decision=…. A marker that is posted but never recognised means a re-review, which is the issue's second symptom. Feed the posted body through the readers' regexes in the test, or have the rewrite put decision= next to sha=.
• Run the prompt snippet instead of grepping it. The cascade-action prompt maps… test only checks the prompt text. Render the template with DECISION=approve and with DECISION=escalate, pass the result to post-pr-review.sh, and check the marker that comes out.
• Escalate path regression. The prompt now writes decision=escalated, not decision=escalate, and no test covers that. Add one escalation case that asserts the new word and that carry-forward / sweep-stuck-reviews detect it.
• Edge cases for the rewrite: two v1 markers in one body (only the first is rewritten), decision=APPROVE in a different case, and decision=approvedX (must stay rejected).

Escalate? no. The rewrite only runs on an approve verdict for the current head sha and cannot upgrade an escalation; the gaps above are about coverage, not unsafe approvals.

Advisory only — I comment, I do not change code. Ground: BMAD Test Architecture
(bmad-tea v1.19.0). Opt out on this item with the qa-lead:hands-off label.

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-10T13:58:29Z.

[donpetry-bot]

No description provided.

[donpetry-bot]

No description provided.

[donpetry-bot]

No description provided.

[donpetry-bot]

No description provided.

[don-petry]

Dev-Lead — fix-bot-comment (no-changes)

Agent reasoning

Looking at this more carefully, the template-drift failure is a **known-red check** under issue #1448. The lint.yml workflow explicitly excludes it from push/merge runs and notes it as "known-red under #1448". The dev-lead.yml deviation is **intentional and tested** — it pins to `dev-lead/v139-ring0` (a promoted tier) instead of `v1-stable` per Safe Release SC2 (#503), and this choice is validated by `tests/test_sc2_self_review_channel.bats`.
Changing this would require rewriting test fixtures and hardcoded references across multiple test files, which violates the "never rewrite tests" guideline. The current PR's changes (to scripts/post-pr-review.sh and related tests for #2169) are unrelated to this pre-existing drift.
**Summary:**
**Bot comment disposition:**
- CodeRabbit comment: **Informational** — purely a rate-limit notice with no code review findings. Dispositioned.
**Tier 1 blocker:**
- template-drift failure: **Known-red under #1448** — the dev-lead.yml drift is intentional (SC2 requirement per #2091, validated by test_sc2_self_review_channel.bats). Not actionable in this PR context. The check is intentionally excluded from merge/push CI per #1486 AC4.
**No code changes required** — all CI checks related to this PR's actual changes (ShellCheck, Lint, bats, CodeRabbit, etc.) are passing. The template-drift and dev-lead CI cancellations are pre-existing infrastructure state.

[don-petry]

No description provided.

[don-petry]

No description provided.

[don-petry]

No description provided.

[don-petry]

No description provided.

[don-petry]

No description provided.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[don-petry]

No description provided.

[donpetry-bot]

No description provided.

[don-petry]

No description provided.

[donpetry-bot]

No description provided.

[don-petry]

CodeAnt assessment is a neutral overview with no specific code defects identified. The PR risk assessment (Low Risk) is informational; all CI checks pass and no blockers remain.

[don-petry]

Dev-Lead — fix-bot-comment (no-changes)

Agent reasoning

Issues addressed: 0
- CodeAnt Low Risk assessment: neutral overview dispositioned as informational [replied + disposition marker]
Files changed: 0
Skipped (informational): 1
```
**Analysis:**
The CodeAnt comment is a **neutral overview** — it reports the PR as "Low Risk" and "safe to merge" without identifying any specific code defects or actionable findings. Per the guidance, such neutral overviews posted as PR issue comments must be dispositioned with `informational` to clear the maintainer-comment gate, even though there is nothing to fix in the diff.
**Disposition:** Posted reply with `<!-- dev-lead:comment-disposition id=IC_kwDOR9SdIs8AAAABa25MVA disposition=informational -->` marker. The harness will minimize the original comment RESOLVED.

[don-petry]

No description provided.

[don-petry]

Dev-Lead — fix-reviews (no-changes)

Agent reasoning

Addressed 0 threads:
Test verification: not run (no changes)
Files changed: none
```

[donpetry-bot]

No description provided.

[don-petry]

This is a rate-limit operational notice from CodeRabbit indicating that the free OSS review quota was reached. The notice contains no code findings or actionable defects — all CI checks pass and there are no Tier 1 blockers. No code changes are needed.

[don-petry]

Dev-Lead — fix-bot-comment (no-changes)

Agent reasoning

---
## Summary
**Bot:** coderabbitai[bot]
**Issues addressed:** 0 (no code findings)
**Disposition:** The CodeRabbit comment is a pure **rate-limit operational notice** indicating the free OSS review quota was reached. It contains no code findings, security architecture review, actionable comments, or any sections with defects. All CI checks pass (36+ successful, 0 failures) and there are no Tier 1 blockers.
**Action taken:** Posted an `informational` disposition reply on the original comment to clear the maintainer-comment gate, as per guidance for non-actionable bot notices.
**Comment URL:** https://github.com/petry-projects/.github-private/pull/2190#issuecomment-6107399548
The PR is ready — no code changes are required.

[donpetry-bot]

Automated review — APPROVED ✓

Risk: MEDIUM
Reviewed commit: 66299e780a6ed17f434323410be2cbb5bb0d8e16
Review mode: triage-approved (single reviewer)

Summary

Fixes the deterministic approval-marker mismatch from #2169. prompts/cascade-action.md now maps the verdict word to the marker vocabulary (approve→approved, escalate→escalated) before running sed. scripts/post-pr-review.sh now normalizes decision=approve to decision=approved, but only inside the current-head-SHA v1 marker and only when the verdict JSON says approve. The #1754 guard is unchanged and still fails closed. Rated MEDIUM (not LOW) because the change sits on the auto-approval gate. It cannot upgrade an escalation, a stale SHA, or a marker that contradicts the verdict.

Linked issue analysis

Closes #2169. The issue's sequencing note asks for acceptance criterion 1 first: a deterministic marker derived from the verdict, a mapped word in cascade-action.md, the #1754 guard still failing closed, and a bats regression test. All of that is done. AC3 (a visible note on post failure) and AC2 (the risk-class policy for tier 3) are left out of this pass on purpose and tracked in follow-up #2189, as the issue asked. The new decision=escalated word in the prompt matches what the consumers already expect (carry-forward.sh:45, review-cycle.sh, review-one-pr.sh).

Findings

No blocking findings.

• Correct: sed -E -z normalization is anchored to sha=${PR_HEAD_SHA}. [^>]* keeps the match inside one marker. The ([[:space:]]|-->) suffix rejects approvedX-style tokens. cmp against temp files avoids false positives from trailing newlines, which resolves the earlier gemini thread. Both prior bot threads are resolved.
• Tests: six new bats cases cover normalization, the decision=approve--> edge, a stale SHA, a contradictory decision=escalate marker, prose-only text (fails closed), and the prompt mapping.
• Non-blocking (carried from the QA-lead advisory): when a model puts risk= before decision=, the posted marker passes the #1754 guard. However, readers that require sha=<SHA>\\s+decision= won't recognise it (carry-forward.sh, review-cycle.sh, pr-review-miss-rate.sh). This ordering gap predates the PR, and the cascade template already puts decision right after sha, so it is a follow-up rather than a blocker. Running the prompt snippet in a test, instead of grepping its text, would also strengthen coverage.
• Note: the issue asks the PR to state that the fix reaches consumers only after the pr-review/v1-next channel tag and rings are promoted. The PR body doesn't say this. It has no effect on correctness.
• Secret-scan MCP tool not available in this run; gitleaks CI check passed.

CI status

Every code-quality and security check passed: Lint, shellcheck/ShellCheck, bats, unit-tests, CodeQL, SonarCloud (Quality Gate passed), gitleaks, actionlint, AgentShield, and the validate-* checks. The dev-lead / dispatch|ci-relay|resume runs show CANCELLED, which is normal concurrency cancellation for that automation and not a code check. The dependency-audit ecosystem jobs were SKIPPED as not applicable. There are no unresolved review threads.

---

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.

[donpetry-bot]

pr-review approved on PARTIAL advisory evidence: 3/5 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596).