Skip to content

feat: implement issue #2160 — GitHub drops about 60% of scheduled ticks here: give the budget poller a second cron slot and a staleness window that matches - #2163

Merged
don-petry merged 7 commits into
mainfrom
dev-lead/issue-2160-20261009-2203
Oct 10, 2026
Merged

don-petry merged 7 commits into
mainfrom
dev-lead/issue-2160-20261009-2203

Conversation

@don-petry

@don-petry don-petry commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

GitHub drops about 60% of scheduled ticks here: give the budget poller a second cron slot and a staleness window that matches

From the issue: The dry-run budget poller (.github/workflows/budget-poller.yml, merged in PR 2031) is scheduled hourly (17 * * * *) and is schedule-only by design (no workflow_dispatch, because a manual dispatch could run branch code that receives the Claude token). Its liveness rule warns when no OK record is newer than bp_stale_hours (default 3 hours), and the retry cron (#2139) ignores a record older than that same bound.

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_budget_poller.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 #2160

View guided diff Turn on auto-fix

…ks here: give the budget poller a second cron slot and a staleness window that matches
@don-petry
don-petry requested a review from a team as a code owner October 9, 2026 22:34
@chatgpt-codex-connector

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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

@coderabbitai

This comment has been minimized.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the dry-run budget poller to run on two hourly cron slots instead of one to address dropped scheduled ticks, adjusts the staleness warning threshold from 3 to 6 hours, and adds tracking for trigger events, cron schedules, and start delays. Feedback suggests using a structural YAML parser like yq instead of raw text scanners (grep and awk) in the tests to ensure robust assertions on the workflow configuration.

Comment thread tests/dev-lead/unit/test_budget_poller.bats
@codeant-ai

codeant-ai Bot commented Oct 9, 2026

Copy link
Copy Markdown

CodeAnt Nitpicks

1 code suggestion

1. With an even number of delay samples, this selects the upper middle value instead of averaging the two middle values, so the reported median is wrong.

Logic error · scripts/lib/budget-poller.sh:368

@codeant-ai

codeant-ai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

CodeAnt PR Risk: Low Risk

  • The PR appears safe to merge; the poller changes are covered by focused tests for scheduling, delivery reporting, and stale-record behavior.
  • The 6-hour staleness window intentionally changes how long consumers trust a poll; tests cover the new default and the existing override.

Assessed commit: 7396ce74ef45

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files

You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread scripts/lib/budget-poller.sh Outdated
Comment thread tests/dev-lead/unit/test_budget_poller.bats Outdated
@donpetry-bot

Copy link
Copy Markdown
Contributor

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-09T23:39:35Z.

…ML with yq

Test-Change-Justification: tests added earlier in this same PR (#2163) rewritten to use yq per reviewer request; no pre-existing test altered.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@don-petry

Copy link
Copy Markdown
Collaborator Author

Dev-Lead — review-changes (applied)

Changes committed and pushed. Requested items addressed:

  • tests/dev-lead/unit/test_budget_poller.bats:704 — applied
  • scripts/lib/budget-poller.sh:368 — applied
  • tests/dev-lead/unit/test_budget_poller.bats:674 — applied

@don-petry
don-petry enabled auto-merge (squash) October 9, 2026 22:50

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread tests/dev-lead/unit/test_budget_poller.bats
@don-petry
don-petry disabled auto-merge October 9, 2026 22:54
@don-petry

Copy link
Copy Markdown
Collaborator Author

Dev-Lead — waiting on PR blockers (intent: fix-reviews)

PR: #2163
No changes were committed, but the PR still can't be marked done: required check CodeQL is still pending. The retry cron will re-attempt automatically. Next attempt after: 2026-10-09T23:25:18Z

@don-petry
don-petry enabled auto-merge (squash) October 9, 2026 22:55
@donpetry-bot

Copy link
Copy Markdown
Contributor

No description provided.

@donpetry-bot

Copy link
Copy Markdown
Contributor

No description provided.

@don-petry
don-petry disabled auto-merge October 9, 2026 23:03
@don-petry

Copy link
Copy Markdown
Collaborator Author

Fixed in scripts/lib/budget-poller.sh line 368: added explicit | floor to array index calculations when computing the median for even-length arrays. This ensures jq treats the indices as integers, preventing the median from incorrectly selecting only the upper middle value instead of averaging the two middle values.

@don-petry don-petry added the ack-test-deletion Acknowledge intentional test deletion (bypasses test-deletion-guard) label Oct 9, 2026
@don-petry

Copy link
Copy Markdown
Collaborator Author

Existing test changed without justification — human attention needed

The fix-bot-comment pass changed, deleted, or skipped existing test(s) to go with its fix: tests/dev-lead/unit/test_budget_poller.bats. A previously-passing test that disagrees with a change is a reason to question the change, not to edit the test, so dev-lead did not push this pass (#2013).

If the test really is wrong, a human should make that call, or the fix should carry a Test-Change-Justification: commit trailer that cites the reason (for example, the review comment that asked for it). Auto-merge has been disabled.

@donpetry-bot

Copy link
Copy Markdown
Contributor

No description provided.

@don-petry

This comment has been minimized.

@don-petry

Copy link
Copy Markdown
Collaborator Author

No description provided.

@donpetry-bot

Copy link
Copy Markdown
Contributor

No description provided.

Test-Change-Justification: poller default bp_stale_hours raised 3h->6h in this PR; maintainer review on #2163 (thread PRRT_kwDOR9SdIs6q_gFL) required the update.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@don-petry

Copy link
Copy Markdown
Collaborator Author

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:

  • scripts/lib/budget-poller.sh:52 — not applied

@don-petry

Copy link
Copy Markdown
Collaborator Author

No description provided.

@donpetry-bot

Copy link
Copy Markdown
Contributor

No description provided.

@don-petry

This comment has been minimized.

@don-petry

Copy link
Copy Markdown
Collaborator Author

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

Agent reasoning
## Summary
**Bot:** coderabbitai[bot]
**Disposition:** `informational` — This is a pure operational notice (rate-limit alert) with no actionable code findings.
**Reason:** The CodeRabbit comment contains only a rate-limit warning stating their free OSS review quota was exhausted. No code defects, security issues, or specific file/line findings are reported. All CI checks pass, and prior reviews confirm issues were addressed.
**Action taken:** Posted disposition reply clearing the maintainer-comment gate. The harness will resolve and minimize the original comment.

@don-petry
don-petry enabled auto-merge October 10, 2026 05:40
@don-petry

Copy link
Copy Markdown
Collaborator Author

No description provided.

@don-petry
don-petry disabled auto-merge October 10, 2026 05:50
…iddle values for even-length arrays

With an even number of delay samples, explicitly floor the division result before
using it as an array index to ensure both middle values are correctly selected and
averaged, rather than selecting only the upper middle value.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@don-petry

Copy link
Copy Markdown
Collaborator Author

Fixed in scripts/lib/budget-poller.sh: explicitly floor the division result before using it as an array index in the even-length case to ensure both middle values are correctly selected and averaged, rather than selecting only the upper middle value.

@don-petry
don-petry enabled auto-merge October 10, 2026 06:00
@sonarqubecloud

Copy link
Copy Markdown

@don-petry

Copy link
Copy Markdown
Collaborator Author

No description provided.

@don-petry
don-petry disabled auto-merge October 10, 2026 06:05
@don-petry

Copy link
Copy Markdown
Collaborator Author

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

Agent reasoning
Addressed 0 new threads. The one open thread was already fixed in a previous pass.
- **Thread PRRT_kwDOR9SdIs6q_gFL** (maintainer thread, `scripts/lib/budget-poller.sh:52`): the fix is in commit d68a6ff. That commit updated `tests/dev-lead/unit/test_dev_lead_retry_budget_hold.bats` for the new 6h stale default and added the two 3h01m-record cases the reviewer asked for. This pass didn't commit anything, so I posted no new reply, addressed-marker or claim. A reply already exists on the thread, and the thread stays open for the maintainer because it has no automation marker.
**Phase 0:** no CI check has failed, and no review is `CHANGES_REQUESTED`. `bats`, `unit`, `auto-rebase-stub` and a few others are still in progress or queued. I didn't check the issue comments (Phase 1b), so any that lack a disposition are unhandled.
**Test verification:** I re-ran `tests/dev-lead/unit/test_dev_lead_retry_budget_hold.bats` locally. The last five tests (32–36) pass. I only looked at the tail of the output, not the full file, and didn't run the full suite.
**Files changed:** none this pass.

@don-petry
don-petry enabled auto-merge October 10, 2026 06:06
@donpetry-bot

Copy link
Copy Markdown
Contributor

No description provided.

@don-petry
don-petry disabled auto-merge October 10, 2026 10:03
@don-petry

Copy link
Copy Markdown
Collaborator Author

CodeRabbit rate-limit notice: the free OSS review quota was exhausted and no code review was performed. There are no actionable findings to address in this comment — all CI checks pass.

@don-petry

Copy link
Copy Markdown
Collaborator Author

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

Agent reasoning
Bot: coderabbitai[bot]
Comment type: Rate-limit notice (operational, non-actionable)
Action: Dispositioned with informational marker
- CodeRabbit review limit exhausted: No code findings present, all CI checks passing — replied with disposition informational [no code changes required]
Files changed: None
Notes: This was a pure operational notice (CodeRabbit rate-limit) with no actionable code findings. The comment was edited after prior disposition attempts, so a fresh disposition was posted to cover the current body. The harness will minimize the original comment RESOLVED.
```
The PR is in good state: **all CI checks pass**, no code changes are required. The bot comment is purely informational about rate limiting and has been properly dispositioned.

@don-petry
don-petry enabled auto-merge October 10, 2026 10:04
donpetry-bot
donpetry-bot previously approved these changes Oct 10, 2026

@donpetry-bot donpetry-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review — APPROVED ✓

Risk: MEDIUM
Reviewed commit: 7396ce74ef45daa9ea6591d8967a7df09922417e
Review mode: triage-approved (single reviewer)

Summary

Adds a second cron slot (:47) to the dry-run budget poller, raises the default staleness window from 3h to 6h (still overridable), and records trigger/scheduled-slot/start-delay fields so tick delivery can be measured. The fleet monitor now reports runs received in 24h against the 48 expected. Schedule-only, read-only permissions, and the single existing secret are all unchanged. The triage-approved assessment holds.

Linked issue analysis

Closes #2160. All four scope items and all four acceptance criteria are addressed:

  • AC1: a yq-based static test asserts exactly two crons, schedule as the only trigger, contents:read,actions:read permissions with no job-level or write scopes, and only CLAUDE_CODE_OAUTH_TOKEN.
  • AC2: tests cover the 6h default and the BUDGET_POLLER_STALE_HOURS override. The dependent retry-cron test in test_dev_lead_retry_budget_hold.bats is updated, with the reason stated in a comment. It also gains FRESH-at-default and STALE-at-3h cases.
  • AC3: bp_build_record tests cover the scheduled fields, previous-hour aliasing, null for non-hourly or invalid crons, an end-to-end run, and carry-forward of pre-#2160 records.
  • AC4: the ShellCheck and bats checks are green.
  • The AGENTS.md rationale for 6h is present, and so is the interaction-model table update.

Findings

No blocking findings.

  • Security: github.event.schedule is passed through env: (BUDGET_POLLER_CRON), not interpolated into run:, so there is no injection surface. No new secret, no new trigger, no permission change. concurrency: budget-poller with cancel-in-progress: false prevents overlap between the two slots.
  • Correctness: the jq scheduled() def safely yields null for non-matching or >59 minute crons, via capture(...)? + select + // null. The median in bp_delivery_line now averages the two middle values for even samples, and a test pins this (600/1200/1800/3600 -> 25m). bp_delivery_line is best-effort (|| true) and handles a missing log.
  • Prior threads: all 5 review threads are resolved. These include the maintainer's required update to the retry-hold test, which was verified at this head. The ack-test-deletion label covers the intentional edits to existing staleness tests (seeds moved from 4h/5h to 7h for the new default).
  • Nit (non-blocking): the retention comment changes from 720 records (~30 days) to ~15 days at two slots an hour. At ~40% delivery the real span will be longer; this is documentation only.

CI status

All required checks pass: Lint, ShellCheck, shellcheck, bats, unit, unit-tests, CodeQL, SonarCloud (quality gate passed), gitleaks, actionlint, AgentShield, and the stub/permissions guards. cubic is NEUTRAL (advisory; it says all reported issues were addressed). Skipped jobs are not applicable to this PR. The MCP secret-scanning tool is not available in this run, but the gitleaks check is green.


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.

@don-petry
don-petry added this pull request to the merge queue Oct 10, 2026
@donpetry-bot
donpetry-bot dismissed their stale review October 10, 2026 10:09

Dismissing approval due to a PR issue comment lacking a verified disposition (#1813)

@donpetry-bot

Copy link
Copy Markdown
Contributor

No description provided.

Merged via the queue into main with commit 0fe8e4c Oct 10, 2026
86 of 89 checks passed
@don-petry
don-petry deleted the dev-lead/issue-2160-20261009-2203 branch October 10, 2026 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ack-test-deletion Acknowledge intentional test deletion (bypasses test-deletion-guard)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GitHub drops about 60% of scheduled ticks here: give the budget poller a second cron slot and a staleness window that matches

2 participants