feat: implement issue #2018 — Wire PR_LIMITS_ORG_CAP org variable into the PR-limit admission gate - #2020
Conversation
…o the PR-limit admission gate
|
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 22 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-02T14:21:37Z. |
Dev-Lead — review-changes (applied)Changes committed and pushed. Requested items addressed:
|
![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 unit tests in test_fix_issue_pr_limit_gate.bats to verify that the PR_LIMITS_ORG_CAP environment variable is correctly wired and mapped across workflow steps. Feedback on these changes highlights two key improvements: first, replacing the unsafe use of mktemp -u with a secure mktemp call and proper error handling to prevent race conditions; second, optimizing the workflow validation loop to avoid performance issues from spawning yq repeatedly and to handle unnamed workflow steps robustly.
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-02T14:29:15Z. |
Superseded by automated re-review at
|
Dev-Lead — fix-reviews (partial)A commit was pushed, but not every requested change was applied. Per requested item:
The unaddressed items above still need work. |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-02T14:41:51Z. |
Superseded by automated re-review at
|
Superseded by automated re-review at
|
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-02T15:06:29Z. |
Resolved — the `needs-human-review` hold was lifted; dev-lead has picked this item up. Click to expand the prior hold notice.dev-lead is withholding action on this item. It is labeled To re-enable automated pickup: remove the |
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:
|
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-02T18:40:51Z. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2f399298d1000009b5b15be335a2f8f423eb5ead
Review mode: triage-approved (single reviewer)
Summary
Adds PR_LIMITS_ORG_CAP: ${{ vars.PR_LIMITS_ORG_CAP }} to the env of both workflow steps that run dev-lead-fix-issue.sh, which calls plg_admission_gate. The steps are in dev-lead-reusable.yml and gh-aw-cross-org.yml. The PR also adds bats coverage for three cases: a set value, an unset value and an empty value. A yq check confirms that every step running the script maps the variable. The variable comes from vars, not secrets, and no run: logic changed. Risk is MEDIUM because the change is to a reusable workflow that 8 downstream repos consume. The one blocking finding from the prior cycle-3 review, the leaked CAP_SEEN_FILE, is fixed at 2f39929.
Linked issue analysis
Closes #2018. Its acceptance criteria are met:
- Every step that invokes the gate passes
vars.PR_LIMITS_ORG_CAP. The only gate caller isscripts/dev-lead-fix-issue.sh, which runs in two workflow steps, and both are covered.initiative-driver.ymldoes not run the gate. - When the variable is unset or empty, the script adds no cap of its own, so behavior is unchanged. Tests cover both cases.
- No cap value is hardcoded, and
pr-limits.jsonis unchanged. - A test covers the env mapping. The yq regression check fails in CI if yq is missing, rather than skipping.
- The guard is fetched at runtime from
petry-projects/.github@main, so nothing vendored needs syncing. It will useplg_effective_org_capautomatically once upstream .github#1221 merges.
Findings
Incremental check against prior review (39aaa6f → 2f39929):
- ✅ Resolved (major, test-hygiene):
CAP_SEEN_FILEleaked when an assertion failed.teardown()now runs[ -z "${CAP_SEEN_FILE:-}" ] || rm -f "$CAP_SEEN_FILE". The cubic thread is resolved. - ℹ️ Carried forward (info, non-blocking): Upstream petry-projects/.github#1221 (
plg_effective_org_cap) is still OPEN. Until it merges, the new env var has no effect, which is harmless because the gate ignores it. When it lands, the resolver must treat set-but-empty as unset, because${{ vars.X }}renders""when the variable is undefined. #2018 states that "an unset or empty variable falls back silently", so the upstream contract already covers this. - ℹ️ Info: The yq check matches steps by a regex on the
run:text, so an indirect invocation through a composite action would not be detected. This is acceptable for now because both current callers invoke the script directly.
New issues: None. The other changes since the prior review come from merging main (#2021) and are not part of this PR's diff.
All 5 review threads (cubic ×3, gemini ×2) are resolved. No human reviewer has asked a question.
CI status
All checks are green at 2f39929, including bats, unit (Test Dev-Lead Agent), shellcheck, actionlint, CodeQL, SonarCloud (Quality Gate passed), gitleaks, AgentShield and Agent Security Scan. Some checks were skipped, which is expected: dependency-audit sub-jobs for ecosystems this repo doesn't use, and the dev-lead ci-relay/resume jobs.
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.
Dismissing approval due to a PR issue comment lacking a verified disposition (#1813)
|
CI checks on this PR are still running. The PR-review sweep re-reviews this PR automatically once the checks complete — no action is needed. Posted by the donpetry-bot PR-review cascade. |
|
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2476110364de882aac91101cc419bd5bff785349
Review mode: triage-approved (single reviewer)
Summary
Maps PR_LIMITS_ORG_CAP: ${{ vars.PR_LIMITS_ORG_CAP }} into the env of both workflow steps that run scripts/dev-lead-fix-issue.sh (the only caller of plg_admission_gate): dev-lead-reusable.yml and gh-aw-cross-org.yml. Adds bats coverage for the set, unset, and set-but-empty cases, plus a yq check that every workflow step running the script maps the variable. The only change since the prior approved review (2f39929) is a merge from main (#1888). That merge touches no file in this PR's diff.
Linked issue analysis
Closes #2018, and the issue is substantively addressed.
- Every gate-invoking step is wired.
git grepat the head SHA showsplg_admission_gateis called only fromscripts/dev-lead-fix-issue.sh. That script is run only by the two workflow steps this PR updates. The yq test enforces this and requires at least 2 matching steps, so a new unmapped caller would fail the test. - No hardcoded cap. The cap comes only from the org variable, with
pr-limits.jsonas the fallback. - Unset or empty leaves behavior unchanged. The script passes the value through untouched, and the tests cover both unset and
"". - Library version. The guard is fetched at runtime from
petry-projects/.github@main(PLG_SOURCE_REFdefaults tomain), not vendored, so nothing needs syncing here. - Heads-up. Upstream petry-projects/.github#1221 is still open. The
maincopy ofscripts/lib/pr-limit-gate.shdoes not yet readPR_LIMITS_ORG_CAP, so setting the org variable has no effect until #1221 merges. After that, it takes effect automatically with no further change in this repo. This wiring is forward-compatible and safe now: the variable is ignored, and admission is unchanged.
Findings
No blocking findings.
- Security.
vars.*is passed only through stepenv:and is never interpolated into arun:script, so there is no expression-injection surface. No secrets are added or changed. - Correctness. The yq selector handles unnamed steps and steps without
run. In CI the regression test fails instead of being skipped when yq is missing.teardowncleans upCAP_SEEN_FILE. - Prior bot threads. All 5 review threads (cubic and gemini) are resolved.
- Informational, non-blocking. The org-cap override stays inert until petry-projects/.github#1221 merges (see the issue analysis above).
CI status
All required checks passed: Lint, shellcheck/ShellCheck, bats, unit-tests, actionlint, CodeQL (actions and python), SonarCloud Quality Gate, gitleaks, AgentShield, and the stub and standards guards. Skipped jobs (dependency-audit ecosystems, dependabot, ci-relay, resume) are not applicable to this PR.
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: 3/5 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596). |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2476110364de882aac91101cc419bd5bff785349
Review mode: triage-approved (single reviewer)
Summary
Adds PR_LIMITS_ORG_CAP: ${{ vars.PR_LIMITS_ORG_CAP }} to the step env of the two workflow steps that run scripts/dev-lead-fix-issue.sh, the only caller of plg_admission_gate. The steps are in dev-lead-reusable.yml and gh-aw-cross-org.yml. The PR also adds bats tests for the set, unset, and set-but-empty cases, plus a yq check that every workflow step running the script maps the variable. This head SHA (2476110) was already approved in the prior cycle. This pass confirms that the diff, CI, and review threads have not changed. Risk is MEDIUM because the change is to a reusable workflow that downstream repos consume.
Linked issue analysis
Closes #2018, and the issue is substantively addressed.
- Every gate-invoking step is wired. Both workflow steps that run
dev-lead-fix-issue.shmap the variable. The yq test enforces this and requires at least 2 matching steps. - No hardcoded cap, and
pr-limits.jsonis unchanged. - Unset or empty leaves behavior unchanged. The value is passed through untouched, and tests cover both unset and
"". - The guard is fetched at runtime from
petry-projects/.github@main, so nothing vendored needs syncing. - Info: #2018 already shows as CLOSED even though this PR is unmerged. It does not block this PR, but the issue may need reopening if this PR does not land.
Findings
No blocking findings.
- Security.
vars.PR_LIMITS_ORG_CAPis used only in stepenv:and is never interpolated into arun:script, so there is no expression-injection surface. No secrets are added or changed. - Correctness and tests. The yq selector handles unnamed steps and steps without
run. In CI the regression check fails instead of being skipped when yq is missing.teardowncleans upCAP_SEEN_FILE, which resolves the leak raised in an earlier cycle. - Threads. All 5 review threads (cubic ×3, gemini ×2) are resolved. No human reviewer has asked a question.
- Info, non-blocking. The override stays inert until upstream petry-projects/.github#1221 merges, and that is forward-compatible. The yq check would not detect an indirect invocation, for example through a composite action. That is acceptable because both current callers invoke the script directly.
CI status
All completed checks pass: Lint, ShellCheck/shellcheck, bats, unit, unit-tests, actionlint, CodeQL (actions and python), SonarCloud Quality Gate, gitleaks, AgentShield, and the stub, standards, and persona guards. Skipped or cancelled jobs (dependency-audit ecosystems, dependabot, superseded dev-lead dispatch/ci-relay/resume runs) do not apply to this PR. The only queued entries are the PR-review jobs for this run.
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
Wire PR_LIMITS_ORG_CAP org variable into the PR-limit admission gate
From the issue: petry-projects/.github#1221 adds a runtime override for the org-wide automation open-PR cap.
plg_effective_org_capinscripts/lib/pr-limit-gate.sh(inpetry-projects/.github) now resolves the cap as: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_fix_issue_pr_limit_gate.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 #2018