fix(prioritize): inline CSMA library into post-script#3182
Conversation
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>
|
🤖 Finished Review · ✅ Success · Started 9:35 PM UTC · Completed 9:40 PM UTC |
PR Summary by QodoFix prioritize by inlining GitHub CSMA helpers into post scripts
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Site previewPreview: https://381e8e32-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1. Invalid top-level return
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # shellcheck shell=bash | ||
|
|
||
| [[ -n "${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0 | ||
| GITHUB_API_CSMA_SH_LOADED=1 |
There was a problem hiding this comment.
1. Invalid top-level return 🐞 Bug ≡ Correctness
post-prioritize.sh (and post-prioritize-test.sh) now contains a top-level return 0 in the CSMA “already loaded” guard, which is invalid when the script is executed (not sourced). If GITHUB_API_CSMA_SH_LOADED is set in the environment, Bash will emit a hard error (`return: can only 'return'...) and may abort under set -euo pipefail`.
Agent Prompt
### Issue description
The inlined CSMA code kept the library-style load guard:
```bash
[[ -n "${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0
GITHUB_API_CSMA_SH_LOADED=1
```
This is valid only when the code is **sourced**. After inlining, it lives in executable scripts and `return` becomes invalid if the variable is pre-set.
### Issue Context
These scripts are executed directly (not sourced), so a pre-set `GITHUB_API_CSMA_SH_LOADED` will trigger an invalid `return` and can break runs.
### Fix Focus Areas
- internal/scaffold/fullsend-repo/scripts/post-prioritize.sh[30-33]
- internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh[284-287]
### Suggested fix
Remove the guard entirely (recommended for inlined code), or replace the `return` with a no-op so it’s always legal in an executable script (e.g., `[[ -n ... ]] && :`) and keep/adjust the sentinel assignment as needed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Review — approvePR: fix(prioritize): inline CSMA library into post-script SummaryThis PR fixes a production regression where every prioritize agent run has been failing since the July 1 migration to base composition (8382e8ba). The root cause is clear: base composition fetches each script as an individual content-addressed blob, so The fix inlines the CSMA library directly into Verified
NotesThe ~315 lines of CSMA logic are now duplicated across two files. This is an acknowledged trade-off: the base composition model doesn't support shared libraries, and other post-scripts already follow the inline pattern. Future CSMA changes need to be applied in both places. No medium or higher findings. The change is a clean, mechanical fix for a real production issue. Labels: PR fixes the prioritize agent post-script and resolves a bug |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
ready-for-merge
ifireball
left a comment
ifireball
left a comment
There was a problem hiding this comment.
Interesting.
Longer term we need to consider how to share code between agent scripts - I would assume we may want the same CSMA-style logic in other scripts, but duplicating it everywhere would be cumbersome.
Do you think we should have an issue for that @ralphbean , @ggallen ?
|
Yes please! Retro agent, you got that, right? |
|
🤖 Finished Retro · ✅ Success · Started 10:39 PM UTC · Completed 10:47 PM UTC |
|
Retro analysis of PR #3182: fix(prioritize): inline CSMA library into post-script. This PR fixed a 5-day production regression where every prioritize agent run failed after the July 1 base composition migration. The fix inlined the CSMA library into post-prioritize scripts. The review agent approved but incorrectly dismissed a latent shell bug (top-level Three proposals filed:
Proposals filed
|
2 participants
Summary
scripts/lib/github-api-csma.shdirectly intopost-prioritize.shandpost-prioritize-test.shscripts/lib/directoryProblem
Base composition (
base:in harness YAML) fetches each script as an individual content-addressed blob. The post-prioritize script doessource "${SCRIPT_DIR}/lib/github-api-csma.sh", butlib/doesn't exist next to the cached blob. This has broken every prioritize run since the July 1 migration to base composition (8382e8ba).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/agents#35
Test plan
bash internal/scaffold/fullsend-repo/scripts/post-prioritize-test.sh— all 7 tests passgo test ./internal/scaffold/...— passesshellcheck— clean