Skip to content

fix(prioritize): CSMA/CD rate-limit resilience for post-prioritize - #1487

Merged
waynesun09 merged 3 commits into
fullsend-ai:mainfrom
ifireball:fix/prioritize-post-csma-cd
May 26, 2026
Merged

fix(prioritize): CSMA/CD rate-limit resilience for post-prioritize#1487
waynesun09 merged 3 commits into
fullsend-ai:mainfrom
ifireball:fix/prioritize-post-csma-cd

Conversation

@ifireball

Copy link
Copy Markdown
Member

Summary

  • Add shared scripts/lib/github-api-csma.sh with carrier-sense quota checks, inter-call jitter, and exponential backoff on GitHub 429/secondary rate limits.
  • Wire the helper into post-prioritize.sh for project field updates and sticky comment posting.
  • Add post-prioritize-test.sh and register it in make script-test.

Test plan

  • bash internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh
  • make script-test
  • go test ./internal/scaffold/... -run TestFullsendRepoFiles

Made with Cursor

@ifireball
ifireball force-pushed the fix/prioritize-post-csma-cd branch from ceaaa26 to a5a1981 Compare May 26, 2026 11:54
Introduce a shared gh rate-limit helper and use it in post-prioritize.sh
so project field updates and comment posting back off on 429/secondary limits.

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Barak Korren <bkorren@redhat.com>
@ifireball
ifireball force-pushed the fix/prioritize-post-csma-cd branch from a5a1981 to 5aa5eb8 Compare May 26, 2026 11:55
@ifireball
ifireball marked this pull request as ready for review May 26, 2026 11:56

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review Squad Report — 6-agent review

Agents: 2x claude-coder, 2x claude-researcher, gemini-code-review, cursor-code-review
Verified findings: 1 CRITICAL, 3 HIGH, 6 MEDIUM (inline), plus 4 LOW / 3 INFO (not inline)
False positives removed: 7

Key finding

The cat "${combined}" bug (lines 201, 242, 279) silently swallows all non-retryable error output — confirmed by 4/6 agents. This should be fixed before merge.

LOW / INFO (not posted inline)

  • LOW: $RANDOM has 15-bit entropy — parallel CI runners with sequential PIDs may produce identical jitter sequences
  • LOW: Shebang on sourced library is misleading (file is mode 100644, designed to be sourced)
  • LOW: >/dev/null on fullsend post-comment suppresses debug output
  • LOW: Tests don't verify RICE score computation or that CSMA was actually invoked
  • INFO: Other post-scripts (triage, code, retro) are unprotected — follow-up work
  • INFO: CSMA/CD analogy is closer to CSMA/CA (WiFi 802.11) — implementation is sound regardless
  • INFO: Test BACKOFF_CAP_SEC=1 prevents exercising real backoff math

Comment thread internal/scaffold/fullsend-repo/scripts/lib/github-api-csma.sh Outdated
Comment thread internal/scaffold/fullsend-repo/scripts/lib/github-api-csma.sh Outdated
Comment thread internal/scaffold/fullsend-repo/scripts/lib/github-api-csma.sh Outdated
Comment thread internal/scaffold/fullsend-repo/scripts/lib/github-api-csma.sh Outdated
Comment thread internal/scaffold/fullsend-repo/scripts/lib/github-api-csma.sh Outdated
Comment thread internal/scaffold/fullsend-repo/scripts/lib/github-api-csma.sh Outdated
Comment thread internal/scaffold/fullsend-repo/scripts/post-prioritize.sh Outdated
Comment thread internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh
ifireball and others added 2 commits May 26, 2026 21:10
Fix error emission, rate-limit detection, backoff timing, and run_cmd
resource parameter; remove redundant slot calls; expand tests.

Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

Copy link
Copy Markdown

Site preview

Preview: https://ec3d15e8-site.fullsend-ai.workers.dev

Commit: a6984f637b547c5211fc2af284f8352785fb03a6

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

No findings.

The CSMA/CD library is well-implemented with proper carrier-sense quota checks, inter-call jitter, and exponential backoff with rate-limit reset awareness. The three wrapper functions (github_csma_run, github_csma_run_pipe, github_csma_run_cmd) correctly handle their respective use cases (direct gh calls, piped stdin to gh, and arbitrary commands). Temp file cleanup via trap ... RETURN is correct for all exit paths.

The integration into post-prioritize.sh correctly wraps all five gh call sites and the fullsend post-comment call. The >/dev/null on the final fullsend call appropriately suppresses success output while preserving stderr for error reporting.

Test coverage is solid: unit tests for github_csma_is_rate_limit (including the {"totalCount":429} false-positive guard) and github_csma_backoff, plus integration tests for happy path, rate-limit retry, non-retryable auth errors, and exhausted retries. The mock gh script correctly simulates rate-limit responses and distinguishes auth errors from retryable failures.

Scaffold bookkeeping is correct: post-prioritize-test.sh added to executableFiles (mode 100755), github-api-csma.sh correctly omitted (mode 100644, sourced not executed), both added to TestFullsendRepoFilesExist, and the test registered in make script-test.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label May 26, 2026

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All 8 review findings addressed in the fix commit. LGTM.

@waynesun09
waynesun09 added this pull request to the merge queue May 26, 2026
Merged via the queue into fullsend-ai:main with commit 86d4838 May 26, 2026
12 checks passed
@github-actions
github-actions Bot deleted the fix/prioritize-post-csma-cd branch June 28, 2026 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants