-
Notifications
You must be signed in to change notification settings - Fork 12
fix(#479): add create-on-missing fallback for ready-for-review label #481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | |||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,60 @@ | |||||||||||||||||||||||
| #!/usr/bin/env bash | |||||||||||||||||||||||
| # labels.lib.sh — Idempotent label creation for fullsend dispatch labels. | |||||||||||||||||||||||
| # | |||||||||||||||||||||||
| # Source from post-scripts: | |||||||||||||||||||||||
| # source "${SCRIPT_DIR}/lib/labels.lib.sh" | |||||||||||||||||||||||
|
|
|||||||||||||||||||||||
| # shellcheck shell=bash | |||||||||||||||||||||||
|
|
|||||||||||||||||||||||
| [[ -n "${LABELS_LIB_SH_LOADED:-}" ]] && return 0 | |||||||||||||||||||||||
| LABELS_LIB_SH_LOADED=1 | |||||||||||||||||||||||
|
|
|||||||||||||||||||||||
| # _label_defaults LABEL — print "description\tcolor" for known labels. | |||||||||||||||||||||||
| # Returns 1 for unknown labels (caller should handle). | |||||||||||||||||||||||
| _label_defaults() { | |||||||||||||||||||||||
| case "$1" in | |||||||||||||||||||||||
| ready-for-review) printf '%s\t%s' 'Fullsend: triggers review agent dispatch' '0E8A16' ;; | |||||||||||||||||||||||
| ready-to-code) printf '%s\t%s' 'Fullsend: triggers code agent dispatch' '0e8a16' ;; | |||||||||||||||||||||||
|
maruiz93 marked this conversation as resolved.
|
|||||||||||||||||||||||
| ready-for-triage) printf '%s\t%s' 'Fullsend: awaiting triage agent' 'ededed' ;; | |||||||||||||||||||||||
| ready-for-merge) printf '%s\t%s' 'Fullsend: all reviewers approved' '0E8A16' ;; | |||||||||||||||||||||||
| requires-manual-review) printf '%s\t%s' 'Fullsend: review requires human judgment' 'FBCA04' ;; | |||||||||||||||||||||||
| rejected) printf '%s\t%s' 'Fullsend: approach rejected by review' 'B60205' ;; | |||||||||||||||||||||||
| needs-human) printf '%s\t%s' 'Fullsend: agent loop needs human input' 'D93F0B' ;; | |||||||||||||||||||||||
| pr-open) printf '%s\t%s' 'Fullsend: open PR addresses this issue' 'D4C5F9' ;; | |||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] premature-decision — incomplete migration leaves two The PR summary lists "No # scripts/pre-code.src.sh:112-114 (bundled: scripts/pre-code.sh:139-141)
gh label create "pr-open" --repo "${REPO_FULL_NAME}" \
--description "An open PR already addresses this issue" --color "D4C5F9" \
--force 2>/dev/null || trueSo the admin-customisation clobber this PR claims to have fixed is still live for The same flip applies to Six inline
Note that Suggested fix: Drop |
|||||||||||||||||||||||
| needs-info) printf '%s\t%s' 'Fullsend: issue needs more information' 'd876e3' ;; | |||||||||||||||||||||||
| blocked) printf '%s\t%s' 'Fullsend: issue blocked on prerequisites' 'e11d48' ;; | |||||||||||||||||||||||
| duplicate) printf '%s\t%s' 'Fullsend: duplicate issue' 'cfd3d7' ;; | |||||||||||||||||||||||
| triaged) printf '%s\t%s' 'Fullsend: triaged, awaiting prioritization' 'c2e0c6' ;; | |||||||||||||||||||||||
| question) printf '%s\t%s' 'Fullsend: issue is a question' 'd876e3' ;; | |||||||||||||||||||||||
| bug) printf '%s\t%s' 'Fullsend: bug report' 'd73a4a' ;; | |||||||||||||||||||||||
| documentation) printf '%s\t%s' 'Fullsend: documentation improvement' '0075ca' ;; | |||||||||||||||||||||||
| feature) printf '%s\t%s' 'Fullsend: feature request' 'a2eeef' ;; | |||||||||||||||||||||||
| not-planned) printf '%s\t%s' 'Fullsend: will not be implemented' 'ffffff' ;; | |||||||||||||||||||||||
| *) return 1 ;; | |||||||||||||||||||||||
| esac | |||||||||||||||||||||||
| } | |||||||||||||||||||||||
|
|
|||||||||||||||||||||||
| # ensure_label REPO LABEL — create a label if it does not already exist. | |||||||||||||||||||||||
| # Uses defaults from _label_defaults when available. No-op when the label | |||||||||||||||||||||||
| # already exists (gh label create returns non-zero for duplicates). | |||||||||||||||||||||||
| # Always returns 0 so callers don't need error handling. | |||||||||||||||||||||||
| ensure_label() { | |||||||||||||||||||||||
| local repo="$1" label="$2" | |||||||||||||||||||||||
| local defaults desc color | |||||||||||||||||||||||
|
maruiz93 marked this conversation as resolved.
|
|||||||||||||||||||||||
| local -a create_args=("$label" --repo "$repo") | |||||||||||||||||||||||
|
|
|||||||||||||||||||||||
| if defaults=$(_label_defaults "$label"); then | |||||||||||||||||||||||
| desc="${defaults%% *}" | |||||||||||||||||||||||
| color="${defaults##* }" | |||||||||||||||||||||||
| create_args+=(--description "$desc" --color "$color") | |||||||||||||||||||||||
| fi | |||||||||||||||||||||||
|
|
|||||||||||||||||||||||
| local err | |||||||||||||||||||||||
| if ! err=$(gh label create "${create_args[@]}" 2>&1); then | |||||||||||||||||||||||
| case "$err" in | |||||||||||||||||||||||
| *already\ exists*) ;; | |||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] premature-decision The duplicate-detection contract is a match on func isLabelAlreadyExistsError(err api.HTTPError) bool {
return err.StatusCode == 422 && len(err.Errors) == 1 &&
err.Errors[0].Field == "name" && err.Errors[0].Code == "already_exists"
}
...
if errors.Is(err, errLabelAlreadyExists) {
return fmt.Errorf("label with name %q already exists; use `--force` to update its color and description", opts.Name)
}So on a duplicate, Two things are unverified, though:
Suggested fix: Either stop parsing prose and use a documented status-code contract — gh api "repos/${repo}/labels/${label}" --silent >/dev/null 2>&1 || gh label create ...— or at minimum match the machine-readable forms too ( This is about how the create result is classified. It is distinct from the settled |
|||||||||||||||||||||||
| *) echo "Warning: gh label create '${label}' failed: ${err}" >&2 ;; | |||||||||||||||||||||||
| esac | |||||||||||||||||||||||
| fi | |||||||||||||||||||||||
| return 0 | |||||||||||||||||||||||
| } | |||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1376,6 +1376,30 @@ run_branch_validation_test "no-agent-target-ignores-allowed-list" \ | |
| run_branch_validation_test "substring-not-accepted" \ | ||
| "release" "main" "release-1,release-2" "reject:release" | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Verify the bundled script uses ensure_label from labels.lib.sh for the | ||
| # ready-for-review label, rather than inline create-on-missing fallback. | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| # Source script must call ensure_label for ready-for-review | ||
| if grep -q 'ensure_label.*ready-for-review' "${POST_SCRIPT}"; then | ||
| echo "PASS: script-calls-ensure-label" | ||
| else | ||
| echo "FAIL: script-calls-ensure-label" | ||
| echo " ${POST_SCRIPT} does not call ensure_label for ready-for-review" | ||
| FAILURES=$((FAILURES + 1)) | ||
| fi | ||
|
|
||
| # Bundled script must have labels.lib.sh inlined (ensure_label + _label_defaults) | ||
| BUNDLED_SCRIPT="${SCRIPT_DIR}/post-code.sh" | ||
| if grep -q '_label_defaults' "${BUNDLED_SCRIPT}" && grep -q 'ensure_label' "${BUNDLED_SCRIPT}"; then | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] no-behavioral-test-coverage Both new assertions are text greps — They would still pass if This matters because the behaviour change in The repo has clear precedent for direct library unit tests — Suggested fix: Add
Also note |
||
| echo "PASS: bundled-has-labels-lib" | ||
| else | ||
| echo "FAIL: bundled-has-labels-lib" | ||
| echo " ${BUNDLED_SCRIPT} missing labels.lib.sh functions" | ||
| FAILURES=$((FAILURES + 1)) | ||
| fi | ||
|
|
||
| # --- Summary --- | ||
|
|
||
| echo "" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ | |
| # | ||
| # Exit codes: | ||
| # 0 — branch pushed and PR created, OR agent determined nothing to do | ||
| # 1 — validation failure or error (nothing pushed) | ||
| # 1 — validation failure, error, or post-push label application failure | ||
| set -euo pipefail | ||
|
|
||
| SCRIPT_DIR_POST="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
|
|
@@ -46,6 +46,8 @@ source "${SCRIPT_DIR_POST}/lib/post-failure-report.lib.sh" | |
| source "${SCRIPT_DIR_POST}/lib/gitleaks-install.lib.sh" | ||
| # shellcheck source=lib/pr-assignee.lib.sh | ||
| source "${SCRIPT_DIR_POST}/lib/pr-assignee.lib.sh" | ||
| # shellcheck source=lib/labels.lib.sh | ||
| source "${SCRIPT_DIR_POST}/lib/labels.lib.sh" | ||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Setup | ||
|
|
@@ -702,9 +704,15 @@ echo "pr_url=${PR_URL}" >> "${GITHUB_OUTPUT:-/dev/null}" | |
| # is used instead (label application requires repo write access). See | ||
| # .github/scripts/check-e2e-authorization-test.sh for trusted-actor rules. | ||
|
maruiz93 marked this conversation as resolved.
|
||
| PR_NUMBER_FROM_URL="${PR_URL##*/}" | ||
| gh issue edit "${PR_NUMBER_FROM_URL}" \ | ||
| ensure_label "${REPO_FULL_NAME}" "ready-for-review" | ||
| label_err="" | ||
| if label_err=$(gh issue edit "${PR_NUMBER_FROM_URL}" \ | ||
| --repo "${REPO_FULL_NAME}" \ | ||
| --add-label "ready-for-review" 2>/dev/null || \ | ||
| gha_echo warning "Failed to apply ready-for-review label to PR #${PR_NUMBER_FROM_URL}" | ||
| --add-label "ready-for-review" 2>&1); then | ||
| echo "Applied ready-for-review label to PR #${PR_NUMBER_FROM_URL}" | ||
| else | ||
| gha_echo error "Failed to apply ready-for-review label to PR #${PR_NUMBER_FROM_URL} — review agent will NOT be dispatched: ${label_err}" | ||
|
maruiz93 marked this conversation as resolved.
|
||
| exit 1 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [high] unreachable-assignment-and-silent-failure
After this change, a label failure leaves a pushed branch and an open PR with no Second problem: Every other fatal path in this file routes through Suggested fix: Move the assignment above the label block (it does not depend on the label), and route the failure through the existing machinery: PR_NUMBER_FROM_URL="${PR_URL##*/}"
maybe_assign_pr "${PR_NUMBER_FROM_URL}"
ensure_label "${REPO_FULL_NAME}" "ready-for-review"
if label_err=$(gh issue edit "${PR_NUMBER_FROM_URL}" --repo "${REPO_FULL_NAME}" \
--add-label "ready-for-review" 2>&1); then
echo "Applied ready-for-review label to PR #${PR_NUMBER_FROM_URL}"
else
post_fail_to_issue label-apply-failed \
"Failed to apply ready-for-review to PR #${PR_NUMBER_FROM_URL} — review agent will NOT be dispatched: ${label_err}"
fiDistinct from the settled |
||
| fi | ||
|
|
||
| maybe_assign_pr "${PR_NUMBER_FROM_URL}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,10 @@ | |
|
|
||
| set -euo pipefail | ||
|
|
||
| SCRIPT_DIR_TRIAGE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
|
maruiz93 marked this conversation as resolved.
|
||
| # shellcheck source=lib/labels.lib.sh | ||
| source "${SCRIPT_DIR_TRIAGE}/lib/labels.lib.sh" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [critical] runtime-source-unavailable
Four independent confirmations:
Why CI is green: Suggested fix: Revert the Independently, add a regression guard in Distinct from the settled |
||
|
|
||
| # Find the triage result JSON — prefer the validated iteration when set. | ||
| # Trust boundary: FULLSEND_VALIDATED_ITERATION_DIR is set by the fullsend CLI | ||
| # on the runner — not by the sandbox or the agent. No containment check | ||
|
|
@@ -74,6 +78,7 @@ echo "Issue: #${ISSUE_NUMBER}" | |
|
|
||
| # add_label uses the labels API to avoid firing issues.edited. | ||
| add_label() { | ||
| ensure_label "${REPO}" "$1" | ||
|
maruiz93 marked this conversation as resolved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [high] mandatory-optional-split-not-implemented Placing Labels now auto-created via direct Driving the script under its own test mock confirms it:
The concrete consequence is the one raised in the open Separately, Suggested fix: Make the split explicit in the library rather than implicit in # Only these labels are auto-created; everything else must pre-exist.
_MANDATORY_LABELS="ready-for-review ready-to-code ready-for-triage"
ensure_label() {
local repo="$1" label="$2"
case " ${_MANDATORY_LABELS} " in *" ${label} "*) ;; *) return 0 ;; esac
...
}Then call Distinct from the settled |
||
| local endpoint="repos/${REPO}/issues/${ISSUE_NUMBER}/labels" | ||
| local err_output | ||
| if ! err_output=$(gh api "${endpoint}" -f "labels[]=$1" --silent 2>&1); then | ||
|
|
@@ -306,9 +311,6 @@ ${FAILED_CREATES}" | |
| remove_label "blocked" | ||
| remove_label "ready-to-code" | ||
| remove_label "needs-info" | ||
| gh label create "pr-open" --repo "${REPO}" \ | ||
| --description "An open PR already addresses this issue" --color "D4C5F9" \ | ||
| --force 2>/dev/null || true | ||
| add_label "pr-open" | ||
| ;; | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.