Skip to content

fix(prioritize): inline CSMA library into post-script#35

Open
ralphbean wants to merge 1 commit into
mainfrom
fix/inline-csma-into-prioritize
Open

fix(prioritize): inline CSMA library into post-script#35
ralphbean wants to merge 1 commit into
mainfrom
fix/inline-csma-into-prioritize

Conversation

@ralphbean

Copy link
Copy Markdown
Member

Summary

  • Inlines scripts/lib/github-api-csma.sh directly into post-prioritize.sh and post-prioritize-test.sh
  • Deletes the now-unused scripts/lib/ directory

Problem

Base composition (base: in harness YAML) fetches each script as an individual content-addressed blob. The post-prioritize script does source "${SCRIPT_DIR}/lib/github-api-csma.sh", but lib/ doesn't exist next to the cached blob. This has broken every prioritize run since the July 1 migration to base composition.

Fix

Inline the CSMA functions, matching the pattern used by other post-scripts (e.g., post-retro.sh handles retries inline rather than sourcing a library).

Companion PR: fullsend-ai/fullsend#3182

Test plan

  • bash scripts/post-prioritize-test.sh — all 7 tests pass
  • shellcheck — clean

The post-prioritize script sources lib/github-api-csma.sh via SCRIPT_DIR,
but base composition fetches scripts as individual content-addressed blobs
without their sibling directories. This breaks every prioritize run under
base composition (since 8382e8ba).

Inline the CSMA functions directly into post-prioritize.sh and
post-prioritize-test.sh, matching the approach other post-scripts use
(e.g., post-retro.sh handles retries inline). Delete the now-unused
scripts/lib/ directory.

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix prioritize: inline GitHub CSMA helpers into post scripts

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Inline GitHub CSMA retry helpers into prioritize post scripts for blob execution.
• Remove scripts/lib dependency that breaks base-composed runs and delete unused library.
• Embed CSMA helpers in prioritize tests to keep them runnable standalone.
Diagram

graph TD
  CACHE["Base composition blob cache"] --> PPS["post-prioritize.sh"] --> CSMA["Inlined CSMA helpers"] --> GH["gh CLI calls"] --> API{{"GitHub API"}}
  CACHE --> PPT["post-prioritize-test.sh"] --> CSMA
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Fix base composition to bundle script directories
  • ➕ Avoids duplicating shared helpers across multiple scripts
  • ➕ Keeps shared library patterns intact (single source of truth)
  • ➖ Requires harness/runtime changes and coordination with the base-composition pipeline
  • ➖ Higher blast radius than a script-only fix
2. Generate a single self-contained script artifact (concat at build time)
  • ➕ Preserves shared library authoring while shipping a one-file runtime artifact
  • ➕ Scales to other scripts that may gain shared helpers later
  • ➖ Adds a build step and artifact management complexity
  • ➖ Makes local development/debugging slightly less direct
3. Vendor the helper as a copied file next to each post-script
  • ➕ Keeps helper in its own file (cleaner than inline) while still blob-safe
  • ➕ Simpler than changing base composition
  • ➖ Still duplicates code across scripts
  • ➖ Relies on adjacent file presence; base composition must fetch both blobs together

Recommendation: Inlining is the most reliable, lowest-blast-radius fix given base composition’s “single blob per script” model and the urgent breakage. Longer term, consider bundling/concatenation so shared helpers remain maintainable without runtime path assumptions.

Files changed (2) +636 / -5

Bug fix (1) +318 / -3
post-prioritize.shInline CSMA GitHub API retry helpers to remove lib/ dependency +318/-3

Inline CSMA GitHub API retry helpers to remove lib/ dependency

• Replaces sourcing scripts/lib/github-api-csma.sh with an inlined copy of the CSMA helper functions. This makes the script runnable when fetched/executed as an isolated content-addressed blob under base composition.

scripts/post-prioritize.sh

Tests (1) +318 / -2
post-prioritize-test.shInline CSMA helpers into prioritize test harness +318/-2

Inline CSMA helpers into prioritize test harness

• Removes the source to scripts/lib/github-api-csma.sh and inlines the same CSMA helper block used by the production post-script. Ensures unit tests remain self-contained and executable without sibling directories.

scripts/post-prioritize-test.sh

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 6, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:05 PM UTC · Completed 10:16 PM UTC
Commit: cac8127 · View workflow run →

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (3)

Context used
✅ Compliance rules (platform): 55 rules
✅ Skills: 4 invoked
  code-review
  code-implementation
  pr-review
  docs-review

Grey Divider


Action required

1. CSMA guard uses return 📜 Skill insight ≡ Correctness
Description
scripts/post-prioritize.sh is executed as a standalone script, but it inlines a CSMA “already
loaded” guard that uses return 0, which is invalid outside a sourced context and can error/abort
unexpectedly under set -e. As a result, when GITHUB_API_CSMA_SH_LOADED is present the guard can
cause the post-prioritize script to fail immediately instead of reliably handling the pre-loaded
CSMA condition.
Code

scripts/post-prioritize.sh[R30-33]

+# shellcheck shell=bash
+
+[[ -n "${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0
+GITHUB_API_CSMA_SH_LOADED=1
Evidence
The cited line in scripts/post-prioritize.sh (`[[ -n "${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return
0) is at top level in an executable script, but return` is only valid inside a function or when a
file is being sourced; when the environment variable is set, that branch triggers and causes a
runtime error rather than a safe early exit. The accompanying test harness evidence indicates the
post script is invoked via bash ... (executed, not sourced), confirming that this return-based
guard does not behave as intended in the actual runtime path it is meant to protect.

scripts/post-prioritize.sh[30-33]
scripts/post-prioritize-test.sh[284-287]
scripts/post-prioritize.sh[13-35]
scripts/post-prioritize-test.sh[249-251]
Skill: code-review

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The inlined CSMA guard in `scripts/post-prioritize.sh` uses `return 0` at script top-level. Because the script is executed (not sourced), `return` is invalid and can cause the script to error/terminate (especially under `set -euo pipefail`) when `GITHUB_API_CSMA_SH_LOADED` is set, meaning the guard does not reliably handle the “CSMA already loaded” scenario.

## Issue Context
The guard previously made sense in the deleted sourced library (`scripts/lib/github-api-csma.sh`), but after inlining into an executable script it should not `return` from the file. The test harness shows `scripts/post-prioritize.sh` (and the related test script) are run via `bash ...`, not `source ...`, so any guard must be compatible with execution semantics and should not short-circuit the entire script incorrectly.

## Fix Focus Areas
- scripts/post-prioritize.sh[30-33]
- scripts/post-prioritize-test.sh[284-287]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. post-retro.sh references deleted file 📜 Skill insight ≡ Correctness
Description
The PR removes the lib/github-api-csma.sh identifier from prioritize scripts, but
scripts/post-retro.sh still references github-api-csma.sh, leaving a stale identifier in the
repository. This violates the requirement to eliminate stale references after an identifier is
removed/retired.
Code

scripts/post-prioritize.sh[L15-17]

-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-# shellcheck source=lib/github-api-csma.sh
-source "${SCRIPT_DIR}/lib/github-api-csma.sh"
+# --- Inlined from lib/github-api-csma.sh ---
+# CSMA/CD-style resilience for GitHub API calls via gh/fullsend.
+# Carrier sense: check rate_limit before transmitting.
+# Slot time: random jitter between calls to desynchronize parallel runners.
+# Collision detection: retry on 429 / secondary rate limit errors with exponential backoff.
+#
+# Environment (all optional):
+#   GITHUB_CSMA_MAX_ATTEMPTS          — default 8
+#   GITHUB_CSMA_MIN_REMAINING_CORE    — default 100
+#   GITHUB_CSMA_MIN_REMAINING_GRAPHQL — default 100
+#   GITHUB_CSMA_SLOT_MIN_MS           — default 250
+#   GITHUB_CSMA_SLOT_MAX_MS           — default 750 (0 disables jitter)
+#   GITHUB_CSMA_SPREAD_MAX_SEC        — default 60 (post-reset desync spread)
+#   GITHUB_CSMA_BACKOFF_CAP_SEC       — default 120
+
+# shellcheck shell=bash
+
+[[ -n "${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0
+GITHUB_API_CSMA_SH_LOADED=1
+
+_github_csma_max_attempts() {
+  echo "${GITHUB_CSMA_MAX_ATTEMPTS:-8}"
+}
+
+_github_csma_min_remaining() {
+  local resource="$1"
+  case "${resource}" in
+    graphql) echo "${GITHUB_CSMA_MIN_REMAINING_GRAPHQL:-100}" ;;
+    *) echo "${GITHUB_CSMA_MIN_REMAINING_CORE:-100}" ;;
+  esac
+}
+
+_github_csma_slot_min_ms() {
+  echo "${GITHUB_CSMA_SLOT_MIN_MS:-250}"
+}
+
+_github_csma_slot_max_ms() {
+  echo "${GITHUB_CSMA_SLOT_MAX_MS:-750}"
+}
+
+_github_csma_spread_max_sec() {
+  echo "${GITHUB_CSMA_SPREAD_MAX_SEC:-60}"
+}
+
+_github_csma_backoff_cap_sec() {
+  echo "${GITHUB_CSMA_BACKOFF_CAP_SEC:-120}"
+}
+
+# Add a random spread delay after a rate-limit sleep to desynchronize runners.
+# Called from both github_csma_sense and _github_csma_sleep_after_rate_limit.
+_github_csma_post_reset_spread() {
+  local spread_max
+  spread_max=$(_github_csma_spread_max_sec)
+  if (( spread_max > 0 )); then
+    local spread_secs=$(( RANDOM % spread_max ))
+    echo "Rate limit reset — spreading ${spread_secs}s to desync from other runners..." >&2
+    sleep "${spread_secs}"
+  fi
+}
+
+_github_csma_emit_failure() {
+  printf '%s\n' "$1" >&2
+}
+
+# Wait until the named rate_limit resource has enough quota (carrier sense).
+# Usage: github_csma_sense [core|graphql] [min_remaining]
+github_csma_sense() {
+  local resource="${1:-core}"
+  local min_remaining="${2:-$(_github_csma_min_remaining "${resource}")}"
+
+  local info remaining reset now wait_secs
+  if ! info=$(gh api rate_limit 2>/dev/null); then
+    echo "WARNING: github_csma_sense: could not read rate_limit; proceeding" >&2
+    return 0
+  fi
+
+  remaining=$(echo "${info}" | jq -r --arg r "${resource}" '.resources[$r].remaining // empty')
+  reset=$(echo "${info}" | jq -r --arg r "${resource}" '.resources[$r].reset // empty')
+
+  if [[ -z "${remaining}" || "${remaining}" == "null" ]]; then
+    echo "WARNING: github_csma_sense: no .resources.${resource} in rate_limit; proceeding" >&2
+    return 0
+  fi
+
+  if (( remaining >= min_remaining )); then
+    return 0
+  fi
+
+  now=$(date +%s)
+  wait_secs=$(( reset - now + 1 ))
+  if (( wait_secs < 1 )); then
+    wait_secs=1
+  fi
+  cap=$(_github_csma_backoff_cap_sec)
+  if (( wait_secs > cap )); then
+    wait_secs="${cap}"
+  fi
+
+  echo "Rate limit sense: ${resource} remaining=${remaining} (min=${min_remaining}); waiting ${wait_secs}s until reset..." >&2
+  sleep "${wait_secs}"
+
+  # After a rate-limit sleep, all runners wake at the same reset timestamp.
+  # Spread them over a wide window to avoid a thundering herd.
+  _github_csma_post_reset_spread
+}
+
+# Random inter-call delay (slot time) to reduce synchronized collisions.
+github_csma_slot() {
+  local max_ms min_ms span_ms delay_ms
+  max_ms=$(_github_csma_slot_max_ms)
+  if (( max_ms <= 0 )); then
+    return 0
+  fi
+  min_ms=$(_github_csma_slot_min_ms)
+  if (( min_ms > max_ms )); then
+    min_ms="${max_ms}"
+  fi
+  span_ms=$(( max_ms - min_ms + 1 ))
+  delay_ms=$(( min_ms + RANDOM % span_ms ))
+  sleep "$(awk -v ms="${delay_ms}" 'BEGIN { printf "%.3f", ms / 1000 }')"
+}
+
+# Return 0 if combined output looks like a retryable GitHub rate limit error.
+github_csma_is_rate_limit() {
+  local text="$1"
+  local lower
+  lower=$(echo "${text}" | tr '[:upper:]' '[:lower:]')
+
+  if echo "${lower}" | grep -qE 'http 429|status: 429'; then
+    return 0
+  fi
+  if echo "${lower}" | grep -qE 'secondary rate limit|rate limit exceeded|api rate limit'; then
+    return 0
+  fi
+  if echo "${lower}" | grep -qE 'http 403|status: 403'; then
+    if echo "${lower}" | grep -qE 'secondary|rate limit|abuse|retry.after'; then
+      return 0
+    fi
+  fi
+  return 1
+}
+
+# Compute backoff seconds for attempt (0-based). Writes to stdout.
+github_csma_backoff() {
+  local attempt="$1"
+  local cap base delay
+  cap=$(_github_csma_backoff_cap_sec)
+  base=$(( 1 << attempt ))
+  if (( base > cap )); then
+    base="${cap}"
+  fi
+  delay=$(( RANDOM % (base + 1) ))
+  if (( delay < 1 )); then
+    delay=1
+  fi
+  echo "${delay}"
+}
+
+_github_csma_sleep_after_rate_limit() {
+  local attempt="$1"
+  local resource="${2:-core}"
+  local delay wait_secs now reset info cap
+
+  delay=$(github_csma_backoff "${attempt}")
+  if info=$(gh api rate_limit 2>/dev/null); then
+    now=$(date +%s)
+    reset=$(echo "${info}" | jq -r --arg r "${resource}" '.resources[$r].reset // empty')
+    if [[ -n "${reset}" && "${reset}" != "null" ]]; then
+      wait_secs=$(( reset - now + 1 ))
+      cap=$(_github_csma_backoff_cap_sec)
+      if (( wait_secs > cap )); then
+        wait_secs="${cap}"
+      fi
+      if (( wait_secs > delay && wait_secs > 0 )); then
+        delay="${wait_secs}"
+      fi
+    fi
+  fi
+  echo "GitHub API rate limit (attempt $(( attempt + 1 ))); backing off ${delay}s..." >&2
+  sleep "${delay}"
+
+  # After backing off, spread runners to avoid thundering herd on wake.
+  _github_csma_post_reset_spread
+}
+
+# Run gh with CSMA/CD. First argument: rate_limit resource (core|graphql).
+# Remaining arguments are passed to gh. Prints gh stdout on success.
+github_csma_run() {
+  local resource="${1:-core}"
+  shift
+
+  local max_attempts attempt outfile errfile combined
+  max_attempts=$(_github_csma_max_attempts)
+  outfile=$(mktemp)
+  errfile=$(mktemp)
+  # shellcheck disable=SC2064
+  trap "rm -f '${outfile}' '${errfile}'" RETURN
+
+  for (( attempt = 0; attempt < max_attempts; attempt++ )); do
+    github_csma_sense "${resource}"
+    github_csma_slot
+
+    : >"${outfile}"
+    : >"${errfile}"
+    local rc=0
+    gh "$@" >"${outfile}" 2>"${errfile}" || rc=$?
+
+    combined=$(cat "${outfile}" "${errfile}")
+    if github_csma_is_rate_limit "${combined}"; then
+      if (( attempt < max_attempts - 1 )); then
+        _github_csma_sleep_after_rate_limit "${attempt}" "${resource}"
+        continue
+      fi
+      _github_csma_emit_failure "${combined}"
+      return 1
+    fi
+
+    if (( rc != 0 )); then
+      _github_csma_emit_failure "${combined}"
+      return 1
+    fi
+    cat "${outfile}"
+    return 0
+  done
+
+  return 1
+}
+
+# Run producer | gh with CSMA/CD. First argument: resource; rest are gh args.
+# Reads producer output from stdin (save once for retries).
+github_csma_run_pipe() {
+  local resource="${1:-graphql}"
+  shift
+
+  local max_attempts attempt infile outfile errfile combined
+  max_attempts=$(_github_csma_max_attempts)
+  infile=$(mktemp)
+  outfile=$(mktemp)
+  errfile=$(mktemp)
+  cat >"${infile}"
+  # shellcheck disable=SC2064
+  trap "rm -f '${infile}' '${outfile}' '${errfile}'" RETURN
+
+  for (( attempt = 0; attempt < max_attempts; attempt++ )); do
+    github_csma_sense "${resource}"
+    github_csma_slot
+
+    : >"${outfile}"
+    : >"${errfile}"
+    local rc=0
+    gh "$@" <"${infile}" >"${outfile}" 2>"${errfile}" || rc=$?
+
+    combined=$(cat "${outfile}" "${errfile}")
+    if github_csma_is_rate_limit "${combined}"; then
+      if (( attempt < max_attempts - 1 )); then
+        _github_csma_sleep_after_rate_limit "${attempt}" "${resource}"
+        continue
+      fi
+      _github_csma_emit_failure "${combined}"
+      return 1
+    fi
+
+    if (( rc != 0 )); then
+      _github_csma_emit_failure "${combined}"
+      return 1
+    fi
+    cat "${outfile}"
+    return 0
+  done
+
+  return 1
+}
+
+# Run an arbitrary command with stdin from caller; retries on rate-limit errors in output.
+# First argument: rate_limit resource (core|graphql); remaining args are the command.
+github_csma_run_cmd() {
+  local resource="${1:-core}"
+  shift
+
+  local max_attempts attempt infile outfile errfile combined
+  max_attempts=$(_github_csma_max_attempts)
+  infile=$(mktemp)
+  outfile=$(mktemp)
+  errfile=$(mktemp)
+  cat >"${infile}"
+  # shellcheck disable=SC2064
+  trap "rm -f '${infile}' '${outfile}' '${errfile}'" RETURN
+
+  for (( attempt = 0; attempt < max_attempts; attempt++ )); do
+    github_csma_sense "${resource}"
+    github_csma_slot
+
+    : >"${outfile}"
+    : >"${errfile}"
+    local rc=0
+    "$@" <"${infile}" >"${outfile}" 2>"${errfile}" || rc=$?
+
+    combined=$(cat "${outfile}" "${errfile}")
+    if github_csma_is_rate_limit "${combined}"; then
+      if (( attempt < max_attempts - 1 )); then
+        _github_csma_sleep_after_rate_limit "${attempt}" "${resource}"
+        continue
+      fi
+      _github_csma_emit_failure "${combined}"
+      return 1
+    fi
+
+    if (( rc != 0 )); then
+      _github_csma_emit_failure "${combined}"
+      return 1
+    fi
+    cat "${outfile}"
+    return 0
+  done
+
+  return 1
+}
+# --- End inlined CSMA ---
Evidence
The diff removes the source "${SCRIPT_DIR}/lib/github-api-csma.sh" identifier from
scripts/post-prioritize.sh, indicating that identifier is no longer valid/used. A separate script
(scripts/post-retro.sh) still references github-api-csma.sh, which is a stale reference after
this PR’s identifier removal.

scripts/post-retro.sh[141-145]
Skill: pr-review

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A remaining repository reference to `github-api-csma.sh` exists after the PR removes the prior `source "${SCRIPT_DIR}/lib/github-api-csma.sh"` usage, leaving stale guidance in another script.

## Issue Context
Even though the remaining reference is in a comment, the compliance rule requires removing all stale references to removed/renamed identifiers across the repo.

## Fix Focus Areas
- scripts/post-retro.sh[141-145]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Protected scripts/ files modified 📜 Skill insight § Compliance
Description
This PR modifies files under scripts/, which is a protected governance/infrastructure path
requiring explicit human review and must not be auto-approved. A compliance finding is required
whenever protected paths are touched.
Code

scripts/post-prioritize.sh[R15-20]

+# --- Inlined from lib/github-api-csma.sh ---
+# CSMA/CD-style resilience for GitHub API calls via gh/fullsend.
+# Carrier sense: check rate_limit before transmitting.
+# Slot time: random jitter between calls to desynchronize parallel runners.
+# Collision detection: retry on 429 / secondary rate limit errors with exponential backoff.
+#
Evidence
The compliance checklist marks scripts/ as a protected path; this PR modifies
scripts/post-prioritize.sh and scripts/post-prioritize-test.sh, so it must be surfaced as a
protected-path change requiring human review.

scripts/post-prioritize.sh[1-20]
scripts/post-prioritize-test.sh[260-275]
Skill: pr-review



Remediation recommended

4. Top-level return in tests 🐞 Bug ☼ Reliability
Description
scripts/post-prioritize-test.sh also includes the inlined CSMA guard `[[ -n
"${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0` at top level, so the test runner will crash if that
env var is set before invocation. This makes the test suite brittle to ambient environment
variables.
Code

scripts/post-prioritize-test.sh[R286-287]

+[[ -n "${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0
+GITHUB_API_CSMA_SH_LOADED=1
Evidence
The test file is intended to be executed directly, yet contains the same top-level return guard,
which will error if the env var is set.

scripts/post-prioritize-test.sh[1-7]
scripts/post-prioritize-test.sh[268-288]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`post-prioritize-test.sh` is an executable script, but it inlines a library-style guard that uses `return 0` at top level. If `GITHUB_API_CSMA_SH_LOADED` is set in the environment, the test script will error immediately.

## Issue Context
This guard was originally designed for a sourced library file. In an executable test script, it should only prevent re-definition of functions, not attempt to return from the script.

## Fix Focus Areas
- scripts/post-prioritize-test.sh[286-287]

### Suggested change
Use the same pattern as the production script fix:

```bash
if [[ -z "${GITHUB_API_CSMA_SH_LOADED:-}" ]]; then
 GITHUB_API_CSMA_SH_LOADED=1
 # define CSMA functions...
fi
```

(or remove the guard entirely).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment on lines +30 to +33
# shellcheck shell=bash

[[ -n "${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0
GITHUB_API_CSMA_SH_LOADED=1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Csma guard uses return 📜 Skill insight ≡ Correctness

scripts/post-prioritize.sh is executed as a standalone script, but it inlines a CSMA “already
loaded” guard that uses return 0, which is invalid outside a sourced context and can error/abort
unexpectedly under set -e. As a result, when GITHUB_API_CSMA_SH_LOADED is present the guard can
cause the post-prioritize script to fail immediately instead of reliably handling the pre-loaded
CSMA condition.
Agent Prompt
## Issue description
The inlined CSMA guard in `scripts/post-prioritize.sh` uses `return 0` at script top-level. Because the script is executed (not sourced), `return` is invalid and can cause the script to error/terminate (especially under `set -euo pipefail`) when `GITHUB_API_CSMA_SH_LOADED` is set, meaning the guard does not reliably handle the “CSMA already loaded” scenario.

## Issue Context
The guard previously made sense in the deleted sourced library (`scripts/lib/github-api-csma.sh`), but after inlining into an executable script it should not `return` from the file. The test harness shows `scripts/post-prioritize.sh` (and the related test script) are run via `bash ...`, not `source ...`, so any guard must be compatible with execution semantics and should not short-circuit the entire script incorrectly.

## Fix Focus Areas
- scripts/post-prioritize.sh[30-33]
- scripts/post-prioritize-test.sh[284-287]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines -15 to +17
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
# shellcheck source=lib/github-api-csma.sh
source "${SCRIPT_DIR}/lib/github-api-csma.sh"
# --- Inlined from lib/github-api-csma.sh ---
# CSMA/CD-style resilience for GitHub API calls via gh/fullsend.
# Carrier sense: check rate_limit before transmitting.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

2. post-retro.sh references deleted file 📜 Skill insight ≡ Correctness

The PR removes the lib/github-api-csma.sh identifier from prioritize scripts, but
scripts/post-retro.sh still references github-api-csma.sh, leaving a stale identifier in the
repository. This violates the requirement to eliminate stale references after an identifier is
removed/retired.
Agent Prompt
## Issue description
A remaining repository reference to `github-api-csma.sh` exists after the PR removes the prior `source "${SCRIPT_DIR}/lib/github-api-csma.sh"` usage, leaving stale guidance in another script.

## Issue Context
Even though the remaining reference is in a comment, the compliance rule requires removing all stale references to removed/renamed identifiers across the repo.

## Fix Focus Areas
- scripts/post-retro.sh[141-145]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +15 to +20
# --- Inlined from lib/github-api-csma.sh ---
# CSMA/CD-style resilience for GitHub API calls via gh/fullsend.
# Carrier sense: check rate_limit before transmitting.
# Slot time: random jitter between calls to desynchronize parallel runners.
# Collision detection: retry on 429 / secondary rate limit errors with exponential backoff.
#

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

3. Protected scripts/ files modified 📜 Skill insight § Compliance

This PR modifies files under scripts/, which is a protected governance/infrastructure path
requiring explicit human review and must not be auto-approved. A compliance finding is required
whenever protected paths are touched.

Comment on lines +286 to +287
[[ -n "${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0
GITHUB_API_CSMA_SH_LOADED=1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

4. Top-level return in tests 🐞 Bug ☼ Reliability

scripts/post-prioritize-test.sh also includes the inlined CSMA guard `[[ -n
"${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0` at top level, so the test runner will crash if that
env var is set before invocation. This makes the test suite brittle to ambient environment
variables.
Agent Prompt
## Issue description
`post-prioritize-test.sh` is an executable script, but it inlines a library-style guard that uses `return 0` at top level. If `GITHUB_API_CSMA_SH_LOADED` is set in the environment, the test script will error immediately.

## Issue Context
This guard was originally designed for a sourced library file. In an executable test script, it should only prevent re-definition of functions, not attempt to return from the script.

## Fix Focus Areas
- scripts/post-prioritize-test.sh[286-287]

### Suggested change
Use the same pattern as the production script fix:

```bash
if [[ -z "${GITHUB_API_CSMA_SH_LOADED:-}" ]]; then
  GITHUB_API_CSMA_SH_LOADED=1
  # define CSMA functions...
fi
```

(or remove the guard entirely).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@fullsend-ai-review

Copy link
Copy Markdown

Review

Verdict: Approve

This PR fixes a production breakage by inlining the CSMA/CD rate-limit resilience library (scripts/lib/github-api-csma.sh) into its two consumers (post-prioritize.sh and post-prioritize-test.sh), then deleting the now-unused library file. The inlined code is a verified verbatim copy of the original. All existing security controls (URL validation, GHA injection protection, HTML sanitization, jq safe interpolation) are untouched. Tests are preserved with identical assertions.

The approach is sound: base composition fetches scripts as individual content-addressed blobs, and source with relative paths cannot resolve adjacent files. Inlining eliminates the dependency on filesystem layout.

Findings

1. Dead source-guard in inlined code · low · correctness
scripts/post-prioritize.sh, scripts/post-prioritize-test.sh

The inlined code carries over [[ -n "${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0 and GITHUB_API_CSMA_SH_LOADED=1. This source-re-entry guard was designed for source'd files where return exits the sourced file. In a directly-executed script under set -euo pipefail, return at the top level would error and abort the script. Currently inert (the variable is never pre-set), but it's dead code that should be removed for hygiene.

Remediation: Remove both the guard line and the GITHUB_API_CSMA_SH_LOADED=1 assignment from both inlined copies — they serve no purpose in inlined code.

2. Code duplication drift risk · low · maintainability
scripts/post-prioritize.sh, scripts/post-prioritize-test.sh

~315 lines of identical CSMA library code now exist in two files with no mechanism to detect divergence. Future bug fixes or enhancements must be applied to both copies.

Remediation: Consider a CI check (e.g., extract the inlined blocks and diff them) to prevent silent drift between the two copies.

3. Stale comment reference to deleted library · low · docs-currency
scripts/post-retro.sh · line 142

Comment reads: "Note: we handle 401/403 inline rather than relying on github-api-csma.sh" — the library no longer exists after this PR. The comment is explanatory (not functional) so this is cosmetic.

Remediation: Update the comment to reference "the CSMA pattern used in post-prioritize.sh" or similar.


Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • scripts/lib/github-api-csma.sh
  • scripts/post-prioritize-test.sh
  • scripts/post-prioritize.sh

Labels: PR fixes a bug in script composition affecting the prioritize agent.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment bug Something isn't working labels Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant