Repository navigation
feat: implement issue #1125 — Fleet pr-review callers pin legacy bare pr-review/stable and are unmanaged — no pr-review release can reach the fleet - #1279
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. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to No concrete issue remains identified that would prevent merging after normal checks. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the legacy pinning and fleet management problem addressed by the changes. It is longer than preferred but remains specific and relevant. |
| Description check | |
The description directly explains the fleet pinning problem, the proposed workflow and deployment changes, testing, risk, rollback, and monitoring. |
| Linked Issues check | |
Issue #1125 requirements are met. standards/workflows/pr-review.yml contains the required header, workflow name, review job, triggers, inputs, permissions, Dependabot guard, reusable call, and mat… |
| Out of Scope Changes check | |
The changes remain within issue #1125. Deployment logic, ring-pin and filename handling, template documentation, SonarQube exemption, and tests support the caller migration and its validation. The cha… |
| Docstring Coverage | |
Docstring coverage is 84.21% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 5 files. (1 skipped: 1 … |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| pull_request_review: | ||
| types: [submitted, dismissed] | ||
| pull_request: | ||
| types: [ready_for_review, reopened, synchronize] |
There was a problem hiding this comment.
Suggestion: Replacing existing callers removes the opened trigger, so newly created PRs do not start a review and may wait indefinitely for another event.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Logic error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** standards/workflows/pr-review.yml
**Line:** 34:34
**Comment:**
*Logic Error: Replacing existing callers removes the `opened` trigger, so newly created PRs do not start a review and may wait indefinitely for another event.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Not changing: dropping the opened trigger is intentional. An existing test asserts the template has no opened trigger, and the engine covers new PRs via check_suite. Per the stub's AGENTS header, trigger types must not change. Left for a human.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not recommend changing the configured trigger types in the PR review workflow; they are constrained by repository instructions and tests.
Applied to:
standards/workflows/pr-review.yml
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| initiative-driver.yml | ||
| pr-auto-review.yml | ||
| feature-ideation.yml | ||
| pr-review.yml |
There was a problem hiding this comment.
Suggestion: Deploying this template removes the legacy pull_request: opened trigger, so a newly opened PR without a completed check suite will not start a review.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Logic error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/deploy-standard-workflows.sh
**Line:** 133:133
**Comment:**
*Logic Error: Deploying this template removes the legacy `pull_request: opened` trigger, so a newly opened PR without a completed check suite will not start a review.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Not changing: same as the template thread. The opened trigger is deliberately absent, and a test asserts it. Left for a human.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag the deliberate absence of the
pull_request: openedtrigger in this workflow; its omission is intentional and covered by a test.
Applied to:
scripts/deploy-standard-workflows.sh
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| if [[ -z "$existing_sha" ]]; then | ||
| skip "$repo/$workflow (replace-if-present: no existing caller — never seeded)" | ||
| continue |
There was a problem hiding this comment.
Suggestion: fetch_existing returns empty for API failures as well as 404s, so a transient read error skips this caller without marking the sweep failed.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Logic error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/deploy-standard-workflows.sh
**Line:** 623:625
**Comment:**
*Logic Error: `fetch_existing` returns empty for API failures as well as 404s, so a transient read error skips this caller without marking the sweep failed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Not changing here: fetch_existing is a shared helper used by every workflow, and changing its error semantics is out of scope for this PR. The failure mode here is safe, because nothing is added or overwritten on a read error. Left for a human.
CodeAnt PR Risk: Medium Risk
Assessed commit: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'replace-if-present' deployment mode for standard workflows, specifically targeting pr-review.yml to prevent seeding it in repositories where it is not already present and protecting the engine host repository (.github-private). It updates the deployment scripts, documentation, and adds comprehensive BATS tests. Review feedback focuses on optimizing Bash performance by replacing here-strings and external commands with native parameter expansions, and improving BATS test reliability by avoiding generic negations and ensuring target files exist before executing grep assertions.
| local -a hosts | ||
| read -r -a hosts <<< "${ENGINE_HOST_REPOS[$w]:-}" |
There was a problem hiding this comment.
Avoid using here-strings (<<<) inside loops or frequently called functions to parse single-line delimited strings in Bash, as it introduces subprocess and setup/teardown overhead on every iteration. Since ENGINE_HOST_REPOS contains simple space-separated values, we can use native Bash parameter expansion and array assignment to parse the string in-process.
| local -a hosts | |
| read -r -a hosts <<< "${ENGINE_HOST_REPOS[$w]:-}" | |
| local -a hosts=(${ENGINE_HOST_REPOS[$w]:-}) |
References
- Avoid using here-strings (
<<<) inside loops to parse single-line delimited strings in Bash, as it introduces subprocess and setup/teardown overhead on every iteration. Use native Bash parameter expansion instead to parse the string in-process.
There was a problem hiding this comment.
Skipping: the suggested unquoted array assignment triggers ShellCheck SC2206 (word-splitting/globbing). read -r -a is the lint-clean form, and the lookup runs once per repo, so the overhead is negligible.
don-petry
left a comment
don-petry
left a comment
There was a problem hiding this comment.
Maintainer note on the two CodeAnt threads that ask for the opened trigger to come back. That suggestion is declined; see the inline comment.
Dev-Lead — review-changes (partial)A commit was pushed, but not every requested change was applied. Per requested item:
The unaddressed items above still need work. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Fixed in sonar-project.properties: added S7637 exemption for pr-review.yml. The new pr-review.yml is a thin caller stub with an intentional first-party mutable channel ref (@pr-review/v1-stable), following the same exemption pattern as other first-party caller stubs (pr-review-mention, pr-auto-review, etc.). This is a required mutable ref per AGENTS.md for the canary-ring release model. |
|
Fixed in sonar-project.properties: registered the s7637_prreview exemption key in the sonar.issue.ignore.multicriteria list. The exemption rule was defined but never registered, so SonarQube reported S7637 violations on pr-review.yml instead of suppressing them. |
|
S7637 exemption for pr-review.yml has been addressed: the exemption was registered in sonar-project.properties multicriteria list in commit ef5ed3f, activating the suppression rule that was previously defined but inactive. |
|
No description provided. |
|
CodeRabbit rate-limit notice with no actionable findings. Recent review explicitly states: 'No actionable comments were generated in the recent review.' All CI checks passing. No code changes required. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
|
No description provided. |
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
… pr-review/stable and are unmanaged — no pr-review release can reach the fleet
The new pr-review.yml is a first-party caller stub with an intentional mutable channel ref (@pr-review/v1-stable), following the same exemption pattern as other first-party caller stubs (pr-review-mention, pr-auto-review, etc). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…ia list The pr-review.yml exemption rule was defined but never registered in the sonar.issue.ignore.multicriteria list, so SonarQube reported S7637 violations instead of suppressing them. Added s7637_prreview to the multicriteria registration to activate the exemption. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
155466b to
04e9a50
Compare
Dev-Lead — rebase (applied)Rebase completed and pushed; PR is no longer conflicting. |
|
No description provided. |
…uplication Replace multiple occurrences of the '.github-private' string literal with a readonly ENGINE_HOST_REPO constant. This reduces code duplication and resolves the SonarQube S1192 issue flagged on this PR. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Fixed in scripts/deploy-standard-workflows.sh: extracted the '.github-private' string literal into a readonly ENGINE_HOST_REPO constant to eliminate duplication and resolve the S1192 string duplication issue. |
|
|
No description provided. |
|
No description provided. |
2 participants
Problem
Fleet pr-review callers pin legacy bare pr-review/stable and are unmanaged — no pr-review release can reach the fleet
From the issue: No pr-review release can reach the fleet through the normal path. Every fleet pr-review caller pins the legacy bare channel
@pr-review/stable. Promotion moves only the major-scopedpr-review/v1-*channels. There is nostandards/workflows/pr-review.ymltemplate andpr-review.ymlis not inDEPLOYABLE_WORKFLOWS, so the sweep never re-pins these callers.Risk
Low — changes automation shell logic under scripts/, covered by shellcheck (--severity=warning) and the bats suite.
Test plan
Tests added/updated:
test/scripts/deploy-standard-workflows/pr-review-replace-if-present.bats,test/scripts/lib/ring-pins.batstest/scripts/standards-templates/vform-pins.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 #1125