fix(#479): add create-on-missing fallback for ready-for-review label - #481
Conversation
ifireball
left a comment
ifireball
left a comment
There was a problem hiding this comment.
Not sure we want this at all (this is a retor-agent suggested change that should've been discussed IMO) - not having certain labels is a way for repo owners to avoid having fullsend set the labels on the repos, though perhaps we should have mandatory and optional labels, where the mandatory ones are the ones used to hand-off work between agents and may be auto-created
b15f90a to
ee2de36
Compare
81ce8c4 to
0b13210
Compare
|
We discussed this in the Jul 29 team sync. The team aligned on two clear groups:
This PR implements that distinction. The shared |
|
🤖 Finished Review · ✅ Success · Started 9:10 AM UTC · Completed 9:28 AM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0b13210 to
50c786e
Compare
|
[error-handling-gap] — Addressed. [fail-open] — By design. The preemptive [behavioral-escalation-risk] — Addressed. [incomplete-migration] — Addressed. Removed the redundant inline [scope-exceeded] — The [protected-path] — Acknowledged. Human review is in progress via @ifireball. [over-engineering] — The 17-label table is intentional. The shared lib covers all known fullsend labels so other scripts can migrate to it incrementally, per the reviewer's "used in all agents" request. [issue-requirement-not-addressed] — The PUSH_TOKEN already documents [inconsistent-label-metadata] — The [source-directive-variable] — Addressed. Renamed |
5cba679 to
aca34a5
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 10:33 AM UTC · Completed 10:51 AM UTC |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
aca34a5 to
9f8c520
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 11:07 AM UTC · Completed 11:24 AM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — 3 agents (Claude x2, Grok), MEDIUM+ findings
Posting only findings that are new — deduped against the 8 existing comment threads and the 3 prior bot review rounds. Every finding below was verified against the code at 9f8c520 before posting; agent-reported findings that turned out to be false positives, already-resolved, or already-declared-intentional were dropped.
6 inline comments posted: 1 critical, 2 high, 3 medium.
The headline is the critical one on scripts/post-triage.sh:24: the source of scripts/lib/labels.lib.sh from a non-bundled runtime script violates the invariant this repo documents in its own README.md:55, and will abort triage on every enrolled repo. CI cannot catch it — check-bundle skips non-BUNDLE_SRCS scripts and shellcheck runs with -e SC1091. That finding and the post-triage.sh:81 one together mean the open CHANGES_REQUESTED is still unresolved: the code does not implement the mandatory/optional split described in the reply to it.
Two additional medium items without a clean inline anchor
[medium] premature-decision — the hardcoded colours contradict the live label set. Scoped to colours only; the description-prefix half of this is already settled as intentional and I'm not reopening it. Against gh api repos/fullsend-ai/agents/labels:
| label | live on this repo | _label_defaults |
|---|---|---|
ready-for-review |
a81af5 |
0E8A16 |
ready-to-code |
ededed |
0e8a16 |
blocked / feature / needs-info / triaged |
all ededed |
e11d48 / a2eeef / d876e3 / c2e0c6 |
No issue, PR comment, or reviewer specified these values — they were chosen, not derived, and nothing pins them. Newly provisioned repos will also get mixed casing for the same green (0e8a16 vs 0E8A16 for the identical colour). Suggest deriving from the existing inline call sites, which are the de-facto spec, or recording where each value came from.
[medium] premature-decision — the token-permission answer cites the wrong token. Issue #479 step 3 asks to verify the coder-role token has Issues write for gh label create. The reply cited PUSH_TOKEN / the workflow permissions: block, but those govern the default GITHUB_TOKEN; the post-script actually receives steps.app-token.outputs.token, minted by .github/actions/mint-token with role: coder, whose permission set lives in the App configuration and cannot be granted or documented by a permissions: block. Probably fine in practice — POST /repos/{o}/{r}/labels and POST .../issues/{n}/labels share the same fine-grained Issues permission, and --add-label already works — but it was reported as verified when it wasn't, and ensure_label degrades a 403 to a warning, so if the coder role is read-only for label creation the symptom is the same silent non-dispatch #479 set out to fix.
Not re-flagged (settled in prior rounds)
scope-exceeded on post-triage.sh; preemptive ensure_label vs 404/422 gating; the 17-label count; the exit-code doc comment; SCRIPT_DIR_TRIAGE naming; the "Fullsend:" description prefix; the swallowed gh label create stderr (resolved in 50c786e).
Where a new finding lands near a settled thread, the inline comment says explicitly how it differs.
Verified as correct (no action)
local err on its own line followed by if ! err=$(...) correctly preserves exit status — no local-masking bug. if defaults=$(_label_defaults ...) is set -e-safe as an if condition. The include guard [[ -n "${LABELS_LIB_SH_LOADED:-}" ]] && return 0 is safe at top level of the bundle when the variable is unset (reproduced). The literal tabs in the %%/## splits are genuinely present and survive bundling intact. create_args is a proper array expanded as "${create_args[@]}", so repo and label names carry no injection risk. make check-bundle passes and post-code.sh is byte-exact.
9f8c520 to
0891505
Compare
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 1:32 AM UTC · Completed 1:50 AM UTC Commit: |
…abels Shared library that creates mandatory dispatch labels (ready-for-review, ready-to-code, ready-for-triage) without --force, preserving admin customizations. Non-mandatory labels are silently skipped. Includes unit tests covering: mandatory create, non-mandatory no-op, already-exists silence, error warning, defaults, REPO fallback, no --force. Signed-off-by: Marta Anon <manon@redhat.com>
…st-code Calls forge_ensure_label before forge_add_label so the label is created on repos where it doesn't exist yet. Fixes silent dispatch failure on newly enrolled repos. Signed-off-by: Marta Anon <manon@redhat.com>
…triage Calls forge_ensure_label before the deferred forge_add_label so ready-to-code is created on repos where it doesn't exist yet. Signed-off-by: Marta Anon <manon@redhat.com>
Preserves admin label customizations by handling 'already exists' silently instead of overwriting with --force on every run. Signed-off-by: Marta Anon <manon@redhat.com>
Use 'true > file' instead of '> file' to clear the calls file, avoiding shellcheck warning about redirections without a command. Signed-off-by: Marta Anon <manon@redhat.com>
936b43b to
207896f
Compare
|
🤖 Finished Review · ❌ Failure · Started 1:54 AM UTC · Completed 1:55 AM UTC Commit: |
Neutralize GHA workflow command sequences (::, %0A, %0D) in error output from gh label create, matching the sanitization pattern used elsewhere in the repo (e.g. SAFE_TITLE in post-retro.sh). Signed-off-by: Marta Anon <manon@redhat.com>
|
🤖 Finished Review · ❌ Failure · Started 2:03 AM UTC · Completed 2:04 AM UTC Commit: |
Rename include guard LABELS_LIB_SH_LOADED → LABELS_SH_LOADED and private function _mandatory_label_defaults → _labels_mandatory_defaults to match the established <STEM>_SH_LOADED and _<lib>_<name> patterns. Signed-off-by: Marta Anon <manon@redhat.com>
|
🤖 Finished Review · ❌ Failure · Started 2:09 AM UTC · Completed 2:10 AM UTC Commit: |
|
🤖 Finished Retro · ✅ Success · Started 9:10 AM UTC · Completed 9:23 AM UTC Commit: |
Retro: PR #481 — create-on-missing fallback for ready-for-review labelLifecycle: 21 days (2026-07-28 to 2026-08-18). Code agent created the PR in 7 minutes, but the entire output was discarded — the branch was 212 commits behind main due to forge abstraction and bundling changes. A human (maruiz93) rewrote the PR from scratch twice after a team sync established the mandatory-vs-optional label design. 8 review rounds total (4 succeeded, 1 cancelled, 3 failed due to infrastructure). Key findings1. Code agent output: 0% survival. All original commits were force-pushed away. Root cause was a stale branch base in a fast-moving repo. This pattern is extensively tracked by existing issues: agents#697, agents#235, agents#565, fullsend#1741, fullsend#3308. 2. Review quality gap — bundling invariant. The automated review produced 17 findings (1 high, 5 medium, 11 low) focused on naming, scope, and documentation. A human Review Squad found 6+ findings (1 critical, 2 high, 3+ medium) focused on behavioral and semantic bugs. The critical finding — 3. Review agent had relevant guidance but didn't apply it deeply. The code-review skill's "Runtime mechanism verification" section (line 89) already says to "trace the full path from where the mechanism is set to where it is read" and check failure paths. The bot found exit-code-contract-violation at LOW but missed the behavioral consequence (unreachable assignment, bypassed ERR trap) that the Review Squad escalated to HIGH. This supports agents#420 (review agent should verify technical assertions against source code). 4. Infrastructure failures. Runs 32089798359, 32090327245, 32090781375 all failed due to sandbox proxy blocking 5. Persistent scope-exceeded re-raising. The review agent flagged scope-exceeded on post-triage changes across 3+ review rounds despite human justification each time. Related to fullsend#3907 (incorporate PR conversation context into review verdict). 6. Design discussion needed pre-coding. ifireball noted this retro-agent-suggested change required team discussion before implementation. agents#605 (human-approval gate before auto-dispatch) would have prevented the wasted code agent run. Proposals filed
|
3 participants
Summary
Fixes #479. Rewrites the original approach from scratch on current main
(original was 212 commits behind with unresolvable conflicts due to
forge abstraction and bundling changes).
scripts/lib/labels.lib.shwithforge_ensure_label()that creates mandatory dispatch labels without--force, preserving admin customizationsready-for-review,ready-to-code,ready-for-triage— auto-created when missingpost-code.src.sh(ready-for-review) andpost-triage.src.sh(ready-to-code) via the shared libpost-retro.sh: removed--forcefromready-for-triagecreation (non-bundled script, can't use shared lib)Design decisions (from team sync 2026-07-29)
auto-created; optional labels fail silently if missing
the enrollment installer
and continues (no exit 1 after push+PR creation)
Follow-up
post-review.shandpost-retro.shto bundled scripts,then migrate to the shared
labels.lib.shTest plan
labels-test.sh: 8 unit tests forforge_ensure_labelpost-code-test.sh: grep check forforge_ensure_labelinbundled script
make test)verify
ready-for-reviewis created and review agent dispatchesdescriptions are not overwritten
🤖 Generated with Claude Code