Repository navigation
feat: implement issue #1017 — [dev-lead timeout A/3] Maximize runtime per stage (raise job + per-tier budgets + guardrail) - #1020
Conversation
… per stage (raise job + per-tier budgets + guardrail)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-07-02T16:23:40Z. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
There was a problem hiding this comment.
Pull request overview
Implements the configuration-only changes for issue #1017 (dev-lead timeout story A/3) by increasing the dev-lead job backstop and raising engine per-tier timeout defaults, with a guardrail script + unit test to prevent the per-tier budgets from exceeding the job cap.
Changes:
- Raise
.github/workflows/dev-lead-reusable.ymldispatch jobtimeout-minutesfrom 60 → 120 with rationale documented. - Increase
scripts/engine.shper-tier default timeouts (especially DEEP/ACTION) and document the fleet impact when repovars.*_TIMEOUT_SECare unset. - Add
scripts/dev-lead-timeout-guardrail.shplus a new Bats unit test to enforceΣ(tier defaults) + overhead ≤ job cap.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/dev-lead/unit/test_timeout_guardrail.bats |
Adds unit tests to enforce the timeout-budget invariant and prove the guardrail fails on an intentionally-invalid configuration. |
scripts/engine.sh |
Raises per-tier default timeouts and documents the invariant math + fleet-wide implication of unset repo vars. |
scripts/dev-lead-timeout-guardrail.sh |
Introduces a CI guardrail that parses engine defaults + workflow job cap and fails if budgets exceed the job backstop. |
.github/workflows/dev-lead-reusable.yml |
Raises the dispatch job timeout to 120 minutes and documents the backstop/guardrail relationship. |
![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 static guardrail script and associated BATS tests to ensure that the sum of per-tier engine timeouts plus a fixed overhead margin does not exceed the workflow's job timeout backstop. It also increases the default timeouts for the DEEP, AUDIT, and ACTION tiers in engine.sh. The review feedback highlights several parsing vulnerabilities in both the guardrail script and the test suite where awk and sed commands do not ignore commented-out lines or handle optional quotes, which could lead to fragile evaluations or script failures under set -e.
…1017) Address Copilot review on #1020: - guardrail: validate every parsed tier budget + job timeout-minutes is a positive integer before the arithmetic, failing with a clear guardrail message (_die) instead of a generic bash arithmetic error if a default is missing or timeout-minutes becomes non-numeric. - test: parse the dispatch timeout-minutes via a _dispatch_timeout_min helper (awk directly, no bash -c wrapper). - test: the Σ+overhead ≤ cap test now recomputes the invariant independently from the parsed config (sum tiers + 900 margin vs job_min*60) rather than grepping the script's own PASS line, then confirms the script agrees. shellcheck clean; 7/7 guardrail bats pass; guardrail still PASS against committed config. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HUhVsBbihbTjVvbcVf1WuU
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Address Gemini review on #1020: harden the engine.sh tier-default (sed) and dispatch timeout-minutes (awk) parsers in both the guardrail script and its test to skip commented-out lines and tolerate leading whitespace, so a stray comment near these keys can never be mis-parsed. (The inputs are anchored at line start today, but a guardrail should parse defensively.) shellcheck clean; 7/7 guardrail bats; guardrail still PASS. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HUhVsBbihbTjVvbcVf1WuU
|
|
@donpetry-bot review |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 16aba726e8be07bc93b05ba2598c2187ab929a90
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
PR raises the dev-lead job backstop 60->120min and per-tier engine budgets (DEEP 600->2400, ACTION 600->2100, AUDIT 600->1200), adding a static guardrail script + bats test that fail CI if Sigma(per-tier budgets)+900s overhead ever exceeds the job cap (currently exact: 7200s=7200s). The gemini findings that drove triage escalation were on the initial commit f82a09f and are already resolved in the current head 472fd55 ('comment-safe, whitespace-tolerant parsers' + numeric validation): the sed/awk parsers skip commented lines and tolerate leading whitespace, and a regex numeric check prevents the arithmetic-expansion error gemini warned of; the suggested '|| true' is unnecessary since sed/awk exit 0 on no-match and explicit -n checks give clearer errors. No secrets, no injection surface (parsers interpolate only script-internal literal var names). Downstream: 8 repos pin dev-lead-reusable.yml but the timeout bump and raised engine defaults are non-breaking (no input/output/secret interface change) so this is informational only. All CI checks pass (the CANCELLED dispatch/ci-relay entries are this PR's own dev-lead agent, not gating); coderabbit approved. MEDIUM risk with all gates satisfied -> approve.
Downstream impact
This change is consumed by 8 downstream repo(s) that pin the affected reusable workflow / lib / prompt. Impacted consumers:
Impacted shared surfaces:
- .github/workflows/dev-lead-reusable.yml
Impacted consumers (8, fetching up to 10):
- petry-projects/.github (pins .github/workflows/dev-lead-reusable.yml)
.github/workflows/dev-lead.yml
- petry-projects/.github-private (pins .github/workflows/dev-lead-reusable.yml)
.github/workflows/dev-lead.yml
- petry-projects/ContentTwin (pins .github/workflows/dev-lead-reusable.yml)
.github/workflows/dev-lead.yml
- petry-projects/TalkTerm (pins .github/workflows/dev-lead-reusable.yml)
.github/workflows/dev-lead.yml
- petry-projects/bmad-bgreat-suite (pins .github/workflows/dev-lead-reusable.yml)
.github/workflows/dev-lead.yml
- petry-projects/broodly (pins .github/workflows/dev-lead-reusable.yml)
.github/workflows/dev-lead.yml
- petry-projects/google-app-scripts (pins .github/workflows/dev-lead-reusable.yml)
.github/workflows/dev-lead.yml
- petry-projects/markets (pins .github/workflows/dev-lead-reusable.yml)
.github/workflows/dev-lead.yml
Findings
- INFO: Sigma(tier budgets)+overhead equals the job cap exactly (7200s == 7200s); the guardrail passes via -le but leaves zero slack, so any future +1s tier bump fails CI. Intentional and documented, but worth noting for whoever next tunes a budget.
- INFO: gemini-code-assist review threads (guardrail:41/55, bats:17) remain unresolved in the GitHub UI but were filed against the initial commit and are substantively addressed by follow-up commits 16aba72 and 472fd55. No code change required.
- INFO: 8 downstream consumers pin dev-lead-reusable.yml (.github, ContentTwin, TalkTerm, bmad-bgreat-suite, broodly, google-app-scripts, markets, .github-private). Change is non-interface-breaking; raised DEEP/ACTION engine defaults apply fleet-wide unless a repo sets its own vars overrides.
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 472fd5559e12a22466ef9f9151b6d0829fcdffcf
Review mode: triage-approved (single reviewer)
Summary
Config-only change per issue #1017: raises the dev-lead dispatch job backstop from 60 to 120 minutes, raises per-tier engine timeout defaults (DEEP 600→2400s, ACTION 600→2100s, AUDIT 600→1200s; TRIAGE/DUCK unchanged at 300s), and adds a static guardrail script plus bats tests asserting Σ(tier budgets) + 900s overhead ≤ job cap. All acceptance criteria met, all validation CI green, all prior bot-review threads resolved by follow-up commits.
Linked issue analysis
Closes #1017 (epic #901, story A/3). All five acceptance criteria are substantively addressed: (1) timeout-minutes 60→120 with in-workflow rationale; (2) DEEP/ACTION raised to their max viable share of the 120-min budget, AUDIT at 1200s within the requested 900–1200 range; (3) guardrail shell check (scripts/dev-lead-timeout-guardrail.sh) + bats test (tests/dev-lead/unit/test_timeout_guardrail.bats) asserting the invariant, including a negative test proving the check fails on an inflated budget; (4) fleet-wide default behavior documented in engine.sh (vars.DEEP_TIMEOUT_SEC unset → engine default applies fleet-wide); (5) shellcheck and existing dev-lead tests green. Scope stays config/workflow-only per the issue's non-goals.
Findings
No blocking findings. Verified: invariant arithmetic is exact (6300s tiers + 900s overhead = 7200s = 120min, ≤ holds at the boundary — intentional and documented); guardrail parsers skip comment lines, tolerate whitespace, and validate all parsed values as positive integers before arithmetic; the negative bats test would also catch sed-pattern rot (an unmatched doctoring sed would leave the guardrail passing and fail the test). Prior copilot findings (bash -c wrapper, self-referential invariant test, unvalidated job_min) and gemini findings (comment-line and quote handling in awk/sed parsers) were all resolved by the two follow-up commits; all 7 review threads are resolved. Secret-scan MCP tool unavailable in this session — noted; gitleaks CI check passed and the diff contains no credential-like content.
CI status
All code-validation checks green: shellcheck (×2), bats, unit-tests, CodeQL, Secret scan (gitleaks), SonarCloud, Lint, agent-shield, holdout-guard, template-drift, prompt-coverage, validate-fixtures, gh-aw-compile, permission/stub checks. Only PENDING entries are this review workflow's own run and dev-lead agent-relay jobs (dispatch/ci-relay), which are orchestration, not code validation. mergeable=MERGEABLE; BLOCKED only on required review.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at 472fd55.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 472fd5559e12a22466ef9f9151b6d0829fcdffcf
Review mode: triage-approved (single reviewer)
Summary
Implements issue #1017 (story A/3 of epic #901): raises the dev-lead dispatch job backstop from 60 to 120 minutes (documented in-workflow, GitHub hard cap 360), raises per-tier engine timeout defaults in scripts/engine.sh (DEEP 600→2400s, ACTION 600→2100s, AUDIT 600→1200s; TRIAGE/DUCK unchanged at 300s), and adds a static guardrail script (scripts/dev-lead-timeout-guardrail.sh) plus bats tests asserting Σ(tier budgets) + 900s overhead ≤ job cap. The invariant is an exact fit: 6300s + 900s = 7200s = 120min, so any further tier raise fails CI loudly — by design. Confirms the triage-tier assessment: config/guardrail change, no behavior or permission changes.
Linked issue analysis
Issue #1017 acceptance criteria are all substantively met: (1) job timeout-minutes raised to 120 with rationale documented in the workflow; (2) DEEP/ACTION defaults raised to the max share that fits the job budget alongside other tiers + 900s fixed overhead; (3) guardrail shell check + bats tests under tests/dev-lead/unit/ assert the Σ+overhead ≤ job-cap invariant, including a negative test proving the guardrail fails when a tier is inflated; (4) fleet note documented in engine.sh that vars.DEEP_TIMEOUT_SEC is unset in petry-projects/.github so the raised engine default applies fleet-wide; (5) shellcheck and existing dev-lead tests green in CI. Non-goals (classification/observability) correctly left out.
Findings
No blocking findings.
- Workflow change is benign: only the dispatch job timeout-minutes value + explanatory comments; no permission, trigger, or action-pinning changes — no GitHub Actions security smells.
- Guardrail script quality: parses config from source-of-truth files (never hardcodes), skips commented-out lines, and validates every parsed value against ^[0-9]+$ with a clear _die message before arithmetic — addressing Copilot's non-numeric-arithmetic concern.
- All 7 review threads resolved: Copilot ×3 and Gemini ×4 findings each have a fix-reply from don-petry and are marked resolved. Minor observation (non-blocking): two replies mention a gsub quote-strip and || true in the awk parsers that are not present in the final head; however the numeric-validation loop covers that failure mode (a quoted timeout value fails loudly with a clear guardrail error rather than a generic bash arithmetic error), so the substance of both findings is addressed.
- Secret scan: run_secret_scanning MCP tool not available in this run; gitleaks CI check passed (SUCCESS), and the diff introduces no credentials or tokens.
- Note: a prior single-reviewer approval already exists at this same head SHA; this triage-approved confirmation reaches the same verdict.
CI status
All checks green at head 472fd55: shellcheck, ShellCheck, bats, unit-tests, CodeQL (actions + python), gitleaks secret scan, SonarCloud, agent-shield, holdout-guard, validate-fixtures, gh-aw-compile, permissions/stub-structure checks — all SUCCESS. CANCELLED entries are superseded duplicate runs of dev-lead/review jobs whose latest runs succeeded. No FAILURE conclusions. reviewDecision: APPROVED.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Closes #1017
Implemented by dev-lead agent. Please review.