Skip to content

feat: allow per-PR fix-loop budget via label #1042

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.

Sign up for GitHub

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

Open
Benkapner wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: allow per-PR fix-loop budget via label #1042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ script-test:
$(call run-timed,bash scripts/pre-review-test.sh)
$(call run-timed,bash scripts/post-review-test.sh)
$(call run-timed,bash scripts/risk-tier1-test.sh)
$(call run-timed,bash scripts/pre-fix-test.sh)
$(call run-timed,bash scripts/post-fix-test.sh)
$(call run-timed,bash scripts/post-retro-test.sh)
$(call run-timed,bash scripts/pre-scribe-test.sh)
Expand Down
15 changes: 13 additions & 2 deletions agents/fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,19 @@ Bot-triggered runs (from the review agent) are capped at `ITERATION_CAP`
(default: 5). When the iteration count approaches this cap, the `needs-human`
label is added and the autonomous loop stops on the next attempt. A human can
then direct the agent with `/fs-fix` commands up to `ITERATION_CAP_HUMAN`
(default: 10) total iterations (bot + human combined). This ensures humans
are never locked out of the agent after a bot loop exhausts its budget.
(default: 10) total iterations (bot + human combined). Because the human cap is
separate from and larger than the bot cap by default, a bot loop exhausting its
budget does not by itself lock a human out (a maintainer can still lower the
human cap explicitly with the label below).

A maintainer can tighten the loop for a single PR with a

@waynesun09 waynesun09 Aug 26, 2026

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.

[MEDIUM] Label tightens the human cap, contradicting the "humans are never locked out" guarantee one paragraph above

Lines 183-187 state the design guarantee verbatim:

A human can then direct the agent with /fs-fix commands up to ITERATION_CAP_HUMAN (default: 10) total iterations (bot + human combined). This ensures humans are never locked out of the agent after a bot loop exhausts its budget.

The paragraph added immediately after says the smallest valid label "lowers whichever cap applies (bot or human) to N", and pre-fix.src.sh:114-118 applies min(budget, CAP) after the bot/human branch, so a fullsend-fix-budget/2 label blocks human /fs-fix at iteration 3 too — asserted by this PR's own test at pre-fix-test.sh:127. The guarantee sentence is now false and is left standing unamended, so the two adjacent paragraphs contradict each other.

This partially overlaps the outdated bot comment on scripts/pre-fix.src.sh:118, which flagged the label as undocumented; at head agents/fix.md is updated, so that thread reads as addressed — the remaining defect is the self-contradiction, which was not reported.

Suggestion. Decide explicitly: either apply the budget to the bot cap only (preserving the invariant), or amend the preceding paragraph to state that a fullsend-fix-budget/N label is the one thing that can lock a human out, and include "remove the fullsend-fix-budget/N label to lift this" in the human-cap escalation message at pre-fix.src.sh:128.

`fullsend-fix-budget/N` label (N a positive integer). If present, the smallest
valid label lowers whichever cap applies (bot or human) to N. The label can
only *tighten* the cap, never raise it: a value at or above the global cap has
no effect, and malformed values (non-integer, zero, negative, or absurdly
large) are ignored so a bad label cannot silently block or widen the loop.
pre-fix enforces the tightened cap and post-fix reports against the same
effective cap (summary and the `needs-human` warning).

## Validation retry behavior

Expand Down
1 change: 1 addition & 0 deletions docs/fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ Remove the label or use `/fs-fix` to re-engage.
|-------|---------|
| `fullsend-no-fix` | Prevents automatic fix runs on this PR. Applied by `/fs-fix-stop`. Manual `/fs-fix` commands are unaffected. |
| `needs-human` | The fix agent is approaching its iteration cap and needs human direction. Applied automatically when an automatic fix iteration reaches the warning threshold. |
| `fullsend-fix-budget/N` | Tightens the review→fix loop for this PR to `N` iterations (`N` a positive integer). Applied by a maintainer. Can only lower the applicable cap (bot or human), never raise it; malformed values are ignored. |

@waynesun09 waynesun09 Aug 27, 2026

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.

[MEDIUM] fullsend-fix-budget/N is documented as a working control although PR_LABELS is populated nowhere in the delivery path

This row states unconditionally that the label "Tightens the review→fix loop for this PR to N iterations ... Applied by a maintainer", with no caveat. But PR_LABELS is set by nothing that reaches the fix agent: harness/fix.yaml:71-78 deliberately omits it from env.runner (correctly, to avoid fail-closed validation), and grepping fullsend-ai/fullsend/.github/workflows shows PR_LABELS exists only in reusable-dispatch.yml:144 (step-scoped to "Determine stage" in a different job) — reusable-fix.yml never sets it. parse_fix_budget therefore always receives the empty string and the feature is inert.

A maintainer following these docs applies fullsend-fix-budget/2 to a risky or expensive PR, sees no error, and gets the full 5-iteration loop anyway — a silent no-op on a cost/safety knob. The PR body's "Scope note" discloses this; the user-facing docs do not.

Relatedly, the pre-existing "Iteration limits" section at docs/fix.md:149-159 still describes only the static 5/10 defaults with no mention that a PR label can lower them — that is the section an operator reads to understand behavior for a specific PR, and it will be incomplete once the wiring lands.

Distinct from the earlier review-body item that asked for this row to exist: the row has now been added, and the residual defect is that it presents the control as live with no caveat. The bot's inertness comment is anchored on scripts/pre-fix.src.sh:116 and is outdated; nothing so far raises that the user-facing docs misrepresent the control as active.

Suggestion. Mark both doc entries as not yet active (e.g. "Reserved — not yet enforced; requires PR_LABELS wiring in reusable-fix.yml") and link a tracking issue, or land the wiring in the same change. Cross-reference the label from the "Iteration limits" section once live. A gha_echo warning in pre-fix when PR_LABELS is unset would also make the dormancy visible in run logs.


## Configuration

Expand Down
8 changes: 8 additions & 0 deletions harness/fix.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ env:
TRIGGER_SOURCE: "${TRIGGER_SOURCE}"
HUMAN_INSTRUCTION: "${HUMAN_INSTRUCTION}"
FIX_ITERATION: "${FIX_ITERATION}"
# NOTE: a `fullsend-fix-budget/N` PR label can tighten the fix cap, and
# pre-fix/post-fix already consume PR_LABELS to enforce it. It is omitted
# here on purpose: the reusable-fix workflow does not yet forward the PR
# labels, and referencing an unset host var fails harness env validation
# (fail-closed) on every run. To activate, re-add
# PR_LABELS: "${PR_LABELS}"
# together with the reusable-fix.yml change that sets PR_LABELS (labels are
# available upstream as a comma-joined string).
REVIEW_BODY_FILE: "${REVIEW_BODY_FILE}"
PRE_AGENT_HEAD: "${PRE_AGENT_HEAD}"
PUSH_TOKEN: "${PUSH_TOKEN}"
Expand Down
52 changes: 52 additions & 0 deletions scripts/lib/fix-budget.lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
#!/usr/bin/env bash

@qodo-code-review qodo-code-review Bot Aug 26, 2026

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

6. Protected scripts require human approval 📜 Skill insight § Compliance

The PR modifies multiple files under the protected scripts/ path, so it must receive human review
and must not be auto-approved. The feature rationale provides context, but there is no linked issue
authorizing these governance/infrastructure changes.
Agent Prompt 
## Issue description
This PR changes protected `scripts/` infrastructure and cannot be auto-approved.

## Issue Context
Route the PR for human approval and link the authorizing issue for the protected-path changes before merge.

## Fix Focus Areas
- scripts/lib/fix-budget.lib.sh[1-42]
- scripts/pre-fix.src.sh[20-29]
- scripts/pre-fix.src.sh[114-119]
- scripts/pre-fix-test.sh[1-59]

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

# shellcheck shell=bash
# fix-budget.lib.sh — parse a per-PR fix-loop budget from PR labels.
#
# A label of the form `fullsend-fix-budget/N` (N a positive integer) lets a
# maintainer cap the review->fix loop for a single PR below the global
# iteration cap. The label can only TIGHTEN the cap, never raise it:
# enforcement lives in pre-fix, which applies min(label_budget, cap).
#
# Bundled into pre-fix.sh via bundle-sh.sh.
#
# Expected env vars (optional):
# PR_LABELS — PR label names separated by commas and/or newlines. Absent/empty
# is fine: parse_fix_budget then returns nothing and the cap is
# unchanged. (The upstream dispatcher comma-joins labels; a
# newline-joined value is also accepted.)

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

FIX_BUDGET_LABEL_PREFIX="fullsend-fix-budget/"

# parse_fix_budget [labels]
# Reads label names (arg 1, or PR_LABELS env when omitted) separated by commas
# and/or newlines. Echoes the smallest valid budget found, or nothing when no
# valid label is present. A malformed value (non-integer, zero, negative) is
# ignored, not fatal — a bad label must not silently drop the existing cap.
parse_fix_budget() {
local labels="${1-${PR_LABELS:-}}"
local best="" label n
# Accept comma-joined labels (the upstream dispatcher format) as well as
# newline-joined: normalize commas to newlines before splitting.
labels="${labels//,/$'\n'}"
while IFS= read -r label; do
Comment on lines +28 to +34

@qodo-code-review qodo-code-review Bot Aug 26, 2026

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

5. Feature lacks linked authorization 📜 Skill insight § Compliance

This PR adds a new parser, runtime guard, generated bundle changes, and tests well beyond the rule's
20-line threshold, but the PR metadata contains no linked authorizing issue. The non-trivial feature
therefore lacks the required explicit authorization.
Agent Prompt 
## Issue description
The non-trivial feature change has no linked issue authorizing its scope.

## Issue Context
Link an issue that explicitly authorizes the per-PR fix-budget feature and confirms the intended producer wiring and enforcement scope.

## Fix Focus Areas
- scripts/lib/fix-budget.lib.sh[1-42]
- scripts/pre-fix.src.sh[114-119]

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

# Trim surrounding whitespace so " fullsend-fix-budget/3 " still matches.
label="${label#"${label%%[![:space:]]*}"}"
label="${label%"${label##*[![:space:]]}"}"
[[ "${label}" == "${FIX_BUDGET_LABEL_PREFIX}"* ]] || continue
n="${label#"${FIX_BUDGET_LABEL_PREFIX}"}"
# Bound the digit count. An arbitrarily long value would overflow Bash's
# signed 64-bit arithmetic in the `-lt` comparison (e.g. 2^64 evaluates as
# 0), which would look "tighter" than any cap and block every fix run.
# A budget above 99999 is meaningless next to caps of 5/10, so treat an
# over-long value as malformed and ignore it.
[[ "${n}" =~ ^[1-9][0-9]{0,4}$ ]] || continue
if [[ -z "${best}" || "${n}" -lt "${best}" ]]; then
best="${n}"
fi
done <<< "${labels}"
[[ -n "${best}" ]] && printf '%s\n' "${best}"
return 0
}
70 changes: 69 additions & 1 deletion scripts/post-fix.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,6 +1206,60 @@ classify_branch_vs_pr_head() {
fi
}
# END bundled: lib/branch-guard.lib.sh
# shellcheck source=lib/fix-budget.lib.sh
# BEGIN bundled: lib/fix-budget.lib.sh
# shellcheck shell=bash
# fix-budget.lib.sh — parse a per-PR fix-loop budget from PR labels.
#
# A label of the form `fullsend-fix-budget/N` (N a positive integer) lets a
# maintainer cap the review->fix loop for a single PR below the global
# iteration cap. The label can only TIGHTEN the cap, never raise it:
# enforcement lives in pre-fix, which applies min(label_budget, cap).
#
# Bundled into pre-fix.sh via bundle-sh.sh.
#
# Expected env vars (optional):
# PR_LABELS — PR label names separated by commas and/or newlines. Absent/empty
# is fine: parse_fix_budget then returns nothing and the cap is
# unchanged. (The upstream dispatcher comma-joins labels; a
# newline-joined value is also accepted.)

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

FIX_BUDGET_LABEL_PREFIX="fullsend-fix-budget/"

# parse_fix_budget [labels]
# Reads label names (arg 1, or PR_LABELS env when omitted) separated by commas
# and/or newlines. Echoes the smallest valid budget found, or nothing when no
# valid label is present. A malformed value (non-integer, zero, negative) is
# ignored, not fatal — a bad label must not silently drop the existing cap.
parse_fix_budget() {
local labels="${1-${PR_LABELS:-}}"
local best="" label n
# Accept comma-joined labels (the upstream dispatcher format) as well as
# newline-joined: normalize commas to newlines before splitting.
labels="${labels//,/$'\n'}"
while IFS= read -r label; do
# Trim surrounding whitespace so " fullsend-fix-budget/3 " still matches.
label="${label#"${label%%[![:space:]]*}"}"
label="${label%"${label##*[![:space:]]}"}"
[[ "${label}" == "${FIX_BUDGET_LABEL_PREFIX}"* ]] || continue
n="${label#"${FIX_BUDGET_LABEL_PREFIX}"}"
# Bound the digit count. An arbitrarily long value would overflow Bash's
# signed 64-bit arithmetic in the `-lt` comparison (e.g. 2^64 evaluates as
# 0), which would look "tighter" than any cap and block every fix run.
# A budget above 99999 is meaningless next to caps of 5/10, so treat an
# over-long value as malformed and ignore it.
[[ "${n}" =~ ^[1-9][0-9]{0,4}$ ]] || continue
if [[ -z "${best}" || "${n}" -lt "${best}" ]]; then
best="${n}"
fi
done <<< "${labels}"
[[ -n "${best}" ]] && printf '%s\n' "${best}"
return 0
}
# END bundled: lib/fix-budget.lib.sh


# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -1544,6 +1598,16 @@ fi
# ---------------------------------------------------------------------------
ITERATION="${FIX_ITERATION:-1}"
BOT_CAP="${ITERATION_CAP:-5}"

# A per-PR `fullsend-fix-budget/N` label may tighten the cap (never raise it).
# pre-fix enforces the tightened cap; mirror it here so the needs-human warning
# and the iteration summary reflect the cap that is actually enforced. Without
# this, a budget of 2 under a global cap of 5 would report "2 of 5" and never
# add needs-human, even though pre-fix rejects the next cycle.
FIX_BUDGET="$(parse_fix_budget "${PR_LABELS:-}")"
if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${BOT_CAP}" ]]; then
BOT_CAP="${FIX_BUDGET}"
fi
WARN_THRESHOLD=$(( BOT_CAP - 1 ))

# The needs-human label is based on the bot cap — it signals that the
Expand All @@ -1568,5 +1632,9 @@ echo " Trigger: ${TRIGGER_SOURCE}"
if is_bot_user "${TRIGGER_SOURCE}"; then
echo " Iteration: ${ITERATION} of ${BOT_CAP} (bot cap)"
else
echo " Iteration: ${ITERATION} of ${ITERATION_CAP_HUMAN:-10} (human cap, total across bot+human)"
HUMAN_CAP="${ITERATION_CAP_HUMAN:-10}"
if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${HUMAN_CAP}" ]]; then
HUMAN_CAP="${FIX_BUDGET}"
fi
echo " Iteration: ${ITERATION} of ${HUMAN_CAP} (human cap, total across bot+human)"
fi
18 changes: 17 additions & 1 deletion scripts/post-fix.src.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ source "${SCRIPT_DIR_POST}/lib/gitleaks-install.lib.sh"
source "${SCRIPT_DIR_POST}/lib/precommit-gate.lib.sh"
# shellcheck source=lib/branch-guard.lib.sh
source "${SCRIPT_DIR_POST}/lib/branch-guard.lib.sh"
# shellcheck source=lib/fix-budget.lib.sh
source "${SCRIPT_DIR_POST}/lib/fix-budget.lib.sh"


# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -403,6 +405,16 @@ fi
# ---------------------------------------------------------------------------
ITERATION="${FIX_ITERATION:-1}"
BOT_CAP="${ITERATION_CAP:-5}"

# A per-PR `fullsend-fix-budget/N` label may tighten the cap (never raise it).
# pre-fix enforces the tightened cap; mirror it here so the needs-human warning
# and the iteration summary reflect the cap that is actually enforced. Without
# this, a budget of 2 under a global cap of 5 would report "2 of 5" and never
# add needs-human, even though pre-fix rejects the next cycle.
FIX_BUDGET="$(parse_fix_budget "${PR_LABELS:-}")"
if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${BOT_CAP}" ]]; then
BOT_CAP="${FIX_BUDGET}"
fi
WARN_THRESHOLD=$(( BOT_CAP - 1 ))

# The needs-human label is based on the bot cap — it signals that the
Expand All @@ -427,5 +439,9 @@ echo " Trigger: ${TRIGGER_SOURCE}"
if is_bot_user "${TRIGGER_SOURCE}"; then
echo " Iteration: ${ITERATION} of ${BOT_CAP} (bot cap)"
else
echo " Iteration: ${ITERATION} of ${ITERATION_CAP_HUMAN:-10} (human cap, total across bot+human)"
HUMAN_CAP="${ITERATION_CAP_HUMAN:-10}"
if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${HUMAN_CAP}" ]]; then
HUMAN_CAP="${FIX_BUDGET}"
fi
echo " Iteration: ${ITERATION} of ${HUMAN_CAP} (human cap, total across bot+human)"
fi
10 changes: 10 additions & 0 deletions scripts/post-review-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,11 @@ is_control_label() {
if [[ "${label}" == risk/* ]]; then
return 0
fi
# Maintainer-set fix-loop budget (fullsend-fix-budget/N); pipeline-managed so
# the review agent preserves it rather than treating it as a contextual label.
if [[ "${label}" == fullsend-fix-budget/* ]]; then

@waynesun09 waynesun09 Aug 27, 2026

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.

[MEDIUM] New control-label tests assert against a copy of is_control_label inside the test file, not the production function

post-review-test.sh defines its own inline is_control_label() at lines 311-328; it never sources post-review.src.sh (POST_SCRIPT at line 386 is only used for bash "${POST_SCRIPT}" end-to-end cases). This PR adds the fullsend-fix-budget/* branch to that duplicate here at lines 322-326, in lockstep with the two production copies (post-review.src.sh:302 and the post-review.sh bundle line 712), so the three new cases verify the test's own mirror.

Proven empirically at head: I deleted the fullsend-fix-budget/* branch from both scripts/post-review.src.sh and scripts/post-review.sh, then ran bash scripts/post-review-test.sh. Result:

PASS: fix-budget-3-is-control
PASS: fix-budget-99999-is-control
PASS: fix-budget-prefix-only-not-control
All tests passed

The tests cannot fail if the production branch is dropped or the copies drift, and this is the only coverage the post-review change gets. (The duplication predates this PR; the PR extends it.)

Suggestion. Extract is_control_label into a sourceable lib — as this PR already did for parse_fix_budget in scripts/lib/fix-budget.lib.sh — and have both post-review.src.sh and the test source it. Either way the assertion must fail when the production branch is removed.

return 0
fi
return 1
}

Expand Down Expand Up @@ -360,8 +365,13 @@ run_control_label_test "risk-elevated-is-control" "risk/elevated" "true"
run_control_label_test "risk-high-is-control" "risk/high" "true"
run_control_label_test "risk-critical-is-control" "risk/critical" "true"

# Maintainer-set fix-budget labels should be control labels
run_control_label_test "fix-budget-3-is-control" "fullsend-fix-budget/3" "true"
run_control_label_test "fix-budget-99999-is-control" "fullsend-fix-budget/99999" "true"

# Non-control labels should NOT be recognized
run_control_label_test "area-api-not-control" "area/api" "false"
run_control_label_test "fix-budget-prefix-only-not-control" "fullsend-fix-budget" "false"
run_control_label_test "priority-high-not-control" "priority/high" "false"
run_control_label_test "bug-not-control" "bug" "false"
run_control_label_test "empty-not-control" "" "false"
Expand Down
5 changes: 5 additions & 0 deletions scripts/post-review.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,11 @@ is_control_label() {
if [[ "${label}" == risk/* ]]; then
return 0
fi
# Maintainer-set fix-loop budget (fullsend-fix-budget/N); pipeline-managed so
# the review agent preserves it rather than treating it as a contextual label.
if [[ "${label}" == fullsend-fix-budget/* ]]; then
return 0
fi
return 1
}

Expand Down
5 changes: 5 additions & 0 deletions scripts/post-review.src.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,11 @@ is_control_label() {
if [[ "${label}" == risk/* ]]; then
return 0
fi
# Maintainer-set fix-loop budget (fullsend-fix-budget/N); pipeline-managed so
# the review agent preserves it rather than treating it as a contextual label.
if [[ "${label}" == fullsend-fix-budget/* ]]; then
return 0
fi
return 1
}

Expand Down
Loading
Loading