feat: allow per-PR fix-loop budget via label - #1042
Conversation
Functional tests did not runFunctional tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the |
PR Summary by QodoAllow per-PR fix-loop budgets via labels
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. PR_LABELS is never populated
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # A per-PR `fullsend-fix-budget/N` label may tighten the cap (never raise it). | ||
| FIX_BUDGET="$(parse_fix_budget "${PR_LABELS:-}")" | ||
| if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${CAP}" ]]; then |
There was a problem hiding this comment.
1. pr_labels is never populated 📜 Skill insight ≡ Correctness
The new cap logic in pre-fix reads PR_LABELS, but neither the checked-in workflow nor the fix harness supplies PR labels through that environment variable, so real runs default to an empty value. Consequently, the advertised fullsend-fix-budget/N label cannot affect normal runtime execution until the missing wiring is added, as the PR description itself acknowledges.
Agent Prompt
## Issue description
The new fix-budget parser is invoked with `PR_LABELS`, but the runtime path never populates that variable, so `fullsend-fix-budget/N` labels cannot affect the fix cap.
## Issue Context
Fetch the current PR/MR's authoritative label names in the workflow and harness dispatch path, covering both supported forges, and pass them as the newline-separated `PR_LABELS` runner environment value expected by `pre-fix`. Preserve the existing empty-value behavior when labels cannot be obtained.
## Fix Focus Areas
- scripts/pre-fix.src.sh[114-119]
- harness/fix.yaml[65-91]
- .github/workflows/fullsend.yaml[22-59]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| parse_fix_budget() { | ||
| local labels="${1-${PR_LABELS:-}}" | ||
| local best="" label n | ||
| while IFS= read -r label; do |
There was a problem hiding this comment.
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
| @@ -0,0 +1,42 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
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
4399fec to
b35ed42
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass on the per-PR fix-loop budget. Four findings are posted inline; three below concern files this PR does not touch, so they have no anchorable diff line.
[HIGH] New fullsend-fix-budget/N control label missing from the post-review control-label denylist
scripts/post-review.src.sh:284
Verified at head. REVIEW_CONTROL_LABELS (post-review.src.sh:284-287) lists only ready-for-merge, requires-manual-review, rejected, ready-for-review, fullsend-no-fix, fullsend-fix, and is_control_label() (289-301) adds exactly one prefix check, risk/*. The new fullsend-fix-budget/N control label is covered by neither.
is_control_label gates the review agent's label_actions at post-review.src.sh:349-352 — the same guard that exists specifically to stop a prompt-injected review from attaching fullsend-no-fix. Two concrete attacks:
add— bounded by thelabel_existscheck at 355, so it only works once a maintainer has created afullsend-fix-budget/Nlabel; but that is precisely the population using this feature, and addingfullsend-fix-budget/1starves the fix loop.remove— the remove branch at 361-363 has nolabel_existsguard at all, so an injected review can strip a maintainer'sfullsend-fix-budget/2label and silently restore the loose global cap, defeating the control entirely.
This is an omission caused by the PR rather than a defect in a changed line, which is why it appears here rather than inline.
Suggestion. Add a fullsend-fix-budget/* prefix check to is_control_label() mirroring the existing risk/* check, and add post-review-test.sh cases asserting both add and remove of a fullsend-fix-budget/N label are refused. Regenerate scripts/post-review.sh with make script-build.
[MEDIUM] New user-facing control label missing from the docs/fix.md control-labels table
docs/fix.md:51
Verified at head. docs/fix.md:46-51 has a "Control labels" table that is the user-facing reference for this agent; it lists exactly two rows, fullsend-no-fix and needs-human, and the surrounding text at 42-44 documents only /fs-fix-stop. This PR adds a third maintainer-applied control label, fullsend-fix-budget/N, but documents it only in agents/fix.md — the prompt shipped into the sandbox, rather than where a maintainer looks. Nothing states that this label, unlike /fs-fix-stop, also blocks manual /fs-fix. A label nobody can discover cannot be used, which compounds the delimiter inertness flagged inline on harness/fix.yaml:71.
Partial overlap with the outdated bot comment on scripts/pre-fix.src.sh:118, whose fix focus named agents/fix.md only — that file is now updated, leaving docs/fix.md as the residual gap.
Suggestion. Add a row to the docs/fix.md "Control labels" table: fullsend-fix-budget/N — caps the review→fix loop at N iterations for this PR; can only lower the global cap, applies to bot and human runs (unlike fullsend-no-fix, which still permits manual /fs-fix), malformed values ignored, removing the label restores the global cap. Also mention it in the iteration-limits section around line 156.
[MEDIUM] No post-fix test that needs-human and the summary honour a tightened cap
scripts/post-fix-test.sh
Verified at head. fix-budget.lib.sh is newly sourced by post-fix.src.sh:69 specifically so post-fix can mirror the tightened cap into WARN_THRESHOLD (post-fix.src.sh:414-418) and the iteration summary (~line 440). But grepping scripts/post-fix-test.sh (1032 lines) for PR_LABELS, budget, or fix-budget yields zero hits — the file was not extended at all. The mirrored-budget path in post-fix is therefore exercised only by the parser unit tests in pre-fix-test.sh, never by an end-to-end assertion that the label changes post-fix behaviour.
Not a duplicate of the existing bot threads: one targeted missing pre-fix enforcement tests (now covered by pre-fix-test.sh:88-127) and another targeted post-fix ignoring the budget entirely (now fixed at head by the mirroring code); the untested post-fix path is what remains.
Related uncovered edge: a fullsend-fix-budget/1 label drives BOT_CAP=1 and WARN_THRESHOLD=0, so the [ "${ITERATION}" -ge "${WARN_THRESHOLD}" ] guard fires on iteration 1 and needs-human is applied after the very first fix run.
Suggestion. Add post-fix-test.sh cases: (a) bot run at iteration 2 with PR_LABELS=fullsend-fix-budget/2 and ITERATION_CAP=5 asserts needs-human is applied and the summary reports "2 of 2", not "2 of 5"; (b) a label value above the global cap has no effect; (c) budget=1 pins the intended iteration-1 needs-human behaviour. Consider clamping WARN_THRESHOLD to a minimum of 1 so the threshold cannot go non-positive if the digit-bound regex is ever loosened.
| # `fullsend-fix-budget/N` label tighten the iteration cap. The dispatcher | ||
| # (reusable-dispatch.yml, upstream fullsend) supplies the value; when it is | ||
| # absent this expands to empty and the cap is unchanged. | ||
| PR_LABELS: "${PR_LABELS}" |
There was a problem hiding this comment.
[CRITICAL] PR_LABELS in env.runner is fail-CLOSED: every fix run aborts at environment validation
Verified against primary sources in fullsend-ai/fullsend. The new comment claims "when it is absent this expands to empty and the cap is unchanged." The opposite is true.
internal/harness/harness.go:685-689documentsValidateRunnerEnvWithas: "Variables set to an empty string are allowed; only truly unset variables produce an error", and its loop overh.Env.Runnerreturnsenv.runner[%s]: host variable %s is not setwhenlookup()reports false.internal/cli/run.go:790callsh.ValidateRunnerEnvWith(lookup)insiderunAgentbefore anyos.Expand, andlookupisos.LookupEnv.TestValidateRunnerEnvWith_ChecksEnvRunner(harness_test.go:529) asserts exactly this for anenv.runnervalue of${MISSING_VAR}.action.yml:415invokesfullsend run "${AGENT}", i.e. that path.
PR_LABELS is genuinely unset there. Grepping the whole repo, PR_LABELS appears only at reusable-dispatch.yml:144 and internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:43, both as step-scoped env on the "Determine stage" step of a different job. The "Run fix agent" step (reusable-fix.yml:359-376) has no PR_LABELS entry, and nothing writes it to GITHUB_ENV.
Every other key in this env.runner block is backed by a step-level env: entry or a GITHUB_ENV write (e.g. GIT_BOT_EMAIL at reusable-fix.yml:164), which GHA sets to empty-string rather than leaving unset — that is why the pattern has worked until now.
The new tests do not catch this: scripts/pre-fix-test.sh:88 sets PR_LABELS explicitly inside env -i, exercising only the script layer, never the harness validation layer. The PR body's "the feature is inert until wired" reasoning does not hold — the harness ships with this PR, so the failure fires on the next agents pin bump with zero workflow changes.
Note: the existing bot comment on scripts/pre-fix.src.sh:116 ("pr_labels is never populated") predates the current head and asserts the opposite failure mode — that runs merely default to empty and the feature is inert. The abort behavior is a distinct, unreported defect in a different file.
Suggestion. Drop the PR_LABELS entry from env.runner in this PR. Both scripts already read ${PR_LABELS:-} straight from the process environment (pre-fix.src.sh:115, post-fix.src.sh:414), so parsing/enforcement works without it. Add the env.runner line in the wiring PR, together with the reusable-fix.yml "Run fix agent" env: entry that guarantees the variable is set-possibly-empty. If it must land now, the workflow wiring has to land in the same change.
| TRIGGER_SOURCE: "${TRIGGER_SOURCE}" | ||
| HUMAN_INSTRUCTION: "${HUMAN_INSTRUCTION}" | ||
| FIX_ITERATION: "${FIX_ITERATION}" | ||
| # Newline-separated PR label names. Consumed by pre-fix/post-fix to let a |
There was a problem hiding this comment.
[MEDIUM] Comment states a dispatcher contract that does not exist, and the delimiter contradicts upstream
Verified against fullsend-ai/fullsend. This comment asserts as fact that "The dispatcher (reusable-dispatch.yml, upstream fullsend) supplies the value", while the PR body says the opposite ("a new optional input that the fix workflow does not populate yet").
reusable-dispatch.yml:144 does define PR_LABELS, but:
- It is step-scoped to "Determine stage" only, never reaching the fix agent step.
- It is comma-joined:
${{ join(github.event.pull_request.labels.*.name, ',') }}— matching the repo-wide convention, sincehas_label()atreusable-dispatch.yml:201-204doesIFS=',' read -ra labels.
parse_fix_budget (fix-budget.lib.sh:29,44) splits on newlines only, via while IFS= read -r label ... <<< "${labels}". If the follow-up wiring reuses the existing dispatcher value — the obvious move given the identical name — the parser silently never matches: bug,fullsend-fix-budget/2,area/api fails the prefix test at line 33, and fullsend-fix-budget/2,area/api fails the ^[1-9][0-9]{0,4}$ bound at line 40. The feature would be permanently inert with no error. GHA expressions also make joining on a literal newline awkward, so the wiring needs deliberate multiline construction rather than a one-line passthrough.
Suggestion. Correct the comment to state the value is not yet supplied. Then either accept the comma delimiter to match the upstream PR_LABELS convention (split on , as well as newline in parse_fix_budget, with comma cases added to pre-fix-test.sh), or rename the input (e.g. PR_LABELS_MULTILINE) so it cannot be confused with the comma-joined upstream variable.
| FIX_BUDGET="$(parse_fix_budget "${PR_LABELS:-}")" | ||
| if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -lt "${CAP}" ]]; then | ||
| gha_echo notice "PR label ${FIX_BUDGET_LABEL_PREFIX}${FIX_BUDGET} tightens the fix cap from ${CAP} to ${FIX_BUDGET}." | ||
| CAP="${FIX_BUDGET}" |
There was a problem hiding this comment.
[MEDIUM] Bot escalation message reports the un-tightened human cap
Anchored at the budget block; the defective message is line 125 (bundled copy: scripts/pre-fix.sh:523).
HUMAN_CAP is assigned from ITERATION_CAP_HUMAN at line 107 and never tightened — the budget tightening at 114-118 writes only to CAP. The bot-branch escalation at line 125 prints:
A human can still direct the agent with /fs-fix (up to ${HUMAN_CAP} total iterations).
With a fullsend-fix-budget/2 label, a bot run at iteration 3 prints "up to 10 total iterations" while the very next human /fs-fix is rejected with "exceeds human cap of 2" — confirmed by this PR's own test at pre-fix-test.sh:127 (run_prefix "alice" 3 ITERATION_CAP_HUMAN 10 $'fullsend-fix-budget/2' expecting "exceeds human cap of 2"). The message actively misleads at the exact moment a maintainer needs accurate guidance.
Suggestion. Apply the budget to both BOT_CAP and HUMAN_CAP up front, before the bot/human branch selects CAP, so the escalation text and the enforced cap cannot diverge; and add "remove the fullsend-fix-budget/N label to lift this" to the message. Regenerate scripts/pre-fix.sh with make script-build and add a test asserting the bot-escalation message names the tightened human cap.
| (default: 10) total iterations (bot + human combined). This ensures humans | ||
| are never locked out of the agent after a bot loop exhausts its budget. | ||
|
|
||
| A maintainer can tighten the loop for a single PR with a |
There was a problem hiding this comment.
[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-fixcommands up toITERATION_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.
Add a fullsend-fix-budget/N PR label that 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: pre-fix applies the budget to both the bot and human caps before selecting one, so the human cap referenced in the bot-escalation message reflects the same effective budget. Parsing lives in a small, pure helper (scripts/lib/fix-budget.lib.sh) that is unit-tested directly (scripts/pre-fix-test.sh) and bundled into pre-fix/post-fix. It accepts both the upstream comma-joined label format and newline-joined input. Malformed label values are ignored rather than fatal, so a bad label never silently drops the existing cap. post-review treats fullsend-fix-budget/* as a pipeline-managed control label so the review agent preserves a maintainer's budget label. The label stays dormant until the reusable-fix workflow forwards PR_LABELS into the fix harness env. fix.yaml deliberately omits the PR_LABELS runner var for now: referencing an unset host var fails harness env validation (fail-closed) on every run, and the workflow does not yet set it. The consumer side is complete and activates by re-adding the runner var together with the workflow change. Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
b35ed42 to
0c6fdef
Compare
|
Thanks for the careful pass. Addressed:
|
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass at head 0c6fdef. Four findings inline: one HIGH on the bot-escalation message, and three MEDIUMs on the docs claim, the post-review test mirror, and the tighten notice. A fifth candidate (no post-fix coverage for the mirrored cap) was dropped as a duplicate of an item already raised on this PR.
|
|
||
| if [[ "${ITERATION}" -gt "${CAP}" ]]; then | ||
| if is_bot_user "${TRIGGER_SOURCE}"; then | ||
| gha_echo error "Fix iteration ${ITERATION} exceeds bot cap of ${CAP}. Escalating to human." |
There was a problem hiding this comment.
[HIGH] Bot-escalation message advertises a /fs-fix recovery path the budget label has already closed, and neither error names the label
Anchored at the bot-escalation branch; the defective sentence is line 131 (last diff line here is 129).
Verified by running scripts/pre-fix.sh at head. The budget is now applied to both caps (lines 113-114), so whenever a budget label is what triggers the escalation, CAP == HUMAN_CAP == FIX_BUDGET and ITERATION already exceeds it. The advertised recovery is therefore dead by construction.
Scenario C (bot, ITERATION=3, ITERATION_CAP=5, ITERATION_CAP_HUMAN=10, PR_LABELS=fullsend-fix-budget/2):
::error::Fix iteration 3 exceeds bot cap of 2. Escalating to human.
::error::A human can still direct the agent with /fs-fix (up to 2 total iterations).
Scenario D (human alice, same inputs) confirms the advice fails immediately:
::error::Fix iteration 3 exceeds human cap of 2.
::error::The /fs-fix loop has run 3 times. Further attempts are blocked.
This strands the maintainer because FIX_ITERATION is cumulative and derived from history, not reset by the label: fullsend-ai/fullsend .github/workflows/reusable-fix.yml:278-282 computes FIX_COMMITS from the PR's fix-authored commits then ITERATION=$((FIX_COMMITS + 1)). So applying fullsend-fix-budget/2 to a PR that already has 3 fix commits instantly blocks every subsequent manual fix command, and neither error message names the label or says to remove it. docs/fix.md does not mention removal either.
Not a duplicate of the existing threads: the outdated pre-fix.src.sh:118 thread asserted the opposite (HUMAN_CAP never tightened) and was addressed by this change, which introduced this new defect; the live agents/fix.md:191 thread covers the documentation contradiction, not the runtime message or the missing recovery instruction. scripts/pre-fix-test.sh:133 ("bot escalation reports tightened human cap", expecting "up to 2 total iterations") cements the misleading text as intended behavior.
Suggestion. Guard the bot-escalation sentence on HUMAN_CAP actually still exceeding ITERATION. When it does not, replace it with text stating the loop is fully exhausted and the fullsend-fix-budget label must be removed. Also make the human-cap error self-describing, e.g. Fix iteration N exceeds human cap of M (set by fullsend-fix-budget/M) — remove the label to continue. Update the pre-fix-test.sh:133 assertion to match, and add the removal instruction to docs/fix.md.
| |-------|---------| | ||
| | `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. | |
There was a problem hiding this comment.
[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.
| 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 |
There was a problem hiding this comment.
[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.
| CAP="${HUMAN_CAP}" | ||
| fi | ||
|
|
||
| if [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -eq "${CAP}" ]]; then |
There was a problem hiding this comment.
[MEDIUM] Tighten notice misses the case it should report and fires on one where the governing cap did not change
The notice guard is [[ -n "${FIX_BUDGET}" && "${FIX_BUDGET}" -eq "${CAP}" ]], comparing only against the cap for the current trigger source. Since line 114 silently tightens HUMAN_CAP on every run regardless of trigger, the notice is wrong in both directions. Confirmed by running scripts/pre-fix.sh at head (ITERATION_CAP=5, ITERATION_CAP_HUMAN=10, bot trigger, iteration 1):
PR_LABELS=fullsend-fix-budget/9→ no notice at all, yet the human cap was silently cut 10→9. This is the half with teeth: a maintainer gets no signal that they reduced the human escape hatch, which is exactly the lockout surface flagged in the HIGH finding on the escalation message.PR_LABELS=fullsend-fix-budget/5→::notice::PR label fullsend-fix-budget/5 caps the fix loop at 5 iteration(s).even though the bot cap was already 5 and did not change; the notice never mentions that the human cap went 10→5.
pre-fix-test.sh covers only budget > cap (asserting no notice, lines 120-127) and budget < cap (notice, line 116). The budget == default-cap case and the human-cap-only-tightened case are both untested.
Suggestion. Track whether each cap actually decreased (e.g. set a flag inside each [[ ... -lt ... ]] branch) and emit a notice naming which caps were lowered and to what — including the human cap when only it changed. Add pre-fix-test.sh cases for budget == default bot cap and for a budget that tightens only the human cap.
Apply the fullsend-fix-budget/N label to the autonomous (bot) cap only, never the human /fs-fix cap. This preserves the documented "humans are never locked out" guarantee and makes the bot-escalation message truthful (the /fs-fix recovery path stays open up to ITERATION_CAP_HUMAN). The escalation error now names the label and how to remove it, and the tighten notice fires only when the bot cap actually decreased (also on human runs). Extract is_control_label + REVIEW_CONTROL_LABELS into a sourceable scripts/lib/review-labels.lib.sh, sourced by both post-review.src.sh and post-review-test.sh, so the control-label tests exercise production code and fail if the fullsend-fix-budget branch drifts (verified). Mirror the bot-cap-only semantics in post-fix summary/needs-human warning. Document the label as reserved/not-yet-active in docs/fix.md and agents/fix.md: PR_LABELS is not forwarded by the review->fix workflow yet, so the label is inert until that wiring lands. Fix the agents/fix.md self-contradiction with the human-lockout guarantee. Add pre-fix enforcement tests: bot-cap tightening, higher/equal labels emit no notice, escalation names the label and keeps the full human cap, and a human run is not blocked by the label. Regenerated bundles via make script-build; check-bundle clean. Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass at head e8cc242. One MEDIUM inline on the activation recipe in harness/fix.yaml; no other new findings.
| # 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}" |
There was a problem hiding this comment.
[MEDIUM] Activation recipe prescribes an env.runner entry that pre-/post-fix do not need, and that would fail closed on GitLab
This comment tells the follow-up author to activate the label by re-adding PR_LABELS: "${PR_LABELS}" to the shared env.runner block together with a reusable-fix.yml change. Verified against fullsend-ai/fullsend main (7495cdc5):
- The entry is redundant for enforcement.
childScriptEnv()ininternal/cli/run.go:3586-3587builds the pre-/post-script environment fromos.Environ()first and only then overlaysRunnerEnv. A step-levelenv: PR_LABELSon the "Run fix agent" step (reusable-fix.yml:361-385) therefore already reaches pre-fix and post-fix — the only two consumers — with noenv.runnerentry at all. - Following the recipe breaks GitLab.
harness/fix.yamlis forge-shared. The GitLab agent template (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml) sets every other key in this shared block (TARGET_BRANCH,HUMAN_INSTRUCTION,REVIEW_BODY_FILE,PRE_AGENT_HEAD,PUSH_TOKEN, ...) but has zero references toPR_LABELS. Adding the entry as written reproduces, on every GitLab fix run, the exactValidateRunnerEnvWithfail-closed abort (harness.go:734-737,env.runner[PR_LABELS]: host variable PR_LABELS is not set) that the earlier thread on this file caught for GitHub. - The forge-scoped blocks already exist —
forge.github.env.runner(~line 119) andforge.gitlab.env.runner(~line 136) are the correct home for any forge-specific entry, and the recipe does not mention them.
This refines the suggestion in the earlier CRITICAL thread (which proposed re-adding the env.runner line in the wiring PR): that step is unnecessary, and unsafe in the shared block.
Suggestion. Rewrite the comment to state that the workflow step env alone is sufficient for pre-/post-fix (childScriptEnv inherits the process environment), so the GitHub wiring is just PR_LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} on the "Run fix agent" step. If an env.runner entry is ever wanted, say it must go in forge.github.env.runner, and in forge.gitlab.env.runner only after the GitLab agent template exports a set-possibly-empty PR_LABELS — never in the shared block.
Step-level env already reaches pre-/post-fix via childScriptEnv; putting PR_LABELS in the shared env.runner block would fail-closed on GitLab. Signed-off-by: Benjamin Kapner <bkapner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass at head cc462ec. Two MEDIUM findings inline: the committed activation recipe in harness/fix.yaml prescribes an expression that is empty in the workflow it names, and the new budget-aware needs-human mirror in post-fix.src.sh has no test coverage. No approval or change request implied.
| # /fs-fix cap is never tightened). No entry is needed in this shared block: | ||
| # childScriptEnv() builds the pre-/post-script env from os.Environ() first, | ||
| # so a step-level env var already reaches both consumers. To activate, set | ||
| # PR_LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} |
There was a problem hiding this comment.
[MEDIUM] Committed activation recipe reads labels from an event context that is never populated in the workflow it names
The NOTE added here (lines 71-80) gives a ready-to-copy recipe: "To activate, set PR_LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} on the 'Run fix agent' step in reusable-fix.yml." Verified against fullsend-ai/fullsend main (ad1037e1105405bcc0737cb2ac8c8e1c3c89f63a): that expression evaluates to the empty string on every run that reaches reusable-fix.yml.
reusable-fix.ymlison: workflow_call, and its only caller is the scaffoldfix.yml(internal/scaffold/fullsend-repo/.github/workflows/fix.yml), which ison: workflow_dispatchand is launched viagh workflow runfromdispatch.yml:560. GitHub's reusable-workflow contract is that thegithubcontext is always the caller's, sogithub.eventthere is aworkflow_dispatchpayload andgithub.event.pull_requestis null.join()on null yields''with no error, so the label would be silently inert forever with no diagnostic.- The obvious fallback cannot supply labels either: the dispatcher deliberately trims
inputs.event_payloadto{issue, pull_request{number,html_url,head,base}, comment}citing injection risk, so no.labelsfield survives. - The recipe also names only one of two wiring sites. The per-repo path does not use
reusable-fix.ymlat all — its "Run fix agent" step is the inlinefix:job atreusable-dispatch.yml:961/1205. Even there the expression is populated only forpull_request_review; onissue_comment(the human/fs-fixpath)github.event.pull_requestis null, so the feature would be dead exactly where a maintainer deliberately applies the budget label.
Confirmed still true at review time: grep PR_LABELS on upstream reusable-fix.yml returns zero hits, and the scaffold fix.yml on: block is still workflow_dispatch. The other halves of the note (childScriptEnv making an env.runner entry unnecessary, and keeping PR_LABELS out of the fail-closed shared block) are correct — only the activation line is wrong. agents/fix.md:201-203 and docs/fix.md:52,164-166 correctly caveat the feature as "Reserved — not yet active" and do not repeat the join() expression, so the defect is contained to this harness note.
Not a duplicate of the existing threads on this file: the thread at line 77 was about the redundant env.runner entry and GitLab fail-closed behaviour (the note was rewritten to absorb it, lines 73-75 and 77-80), and the thread at line 71 was about the newline-vs-comma delimiter, since fixed in the parser. Neither observes that the prescribed expression is empty in the target workflow.
Suggestion. Rewrite the activation line to say labels are not reachable from github.event or inputs.event_payload in the workflow_dispatch-launched fix path, and give the pattern reusable-fix.yml already uses for HEAD_REF on issue_comment: in the "Extract PR number and context" step (reusable-fix.yml:201), resolve labels via
gh api "repos/${SOURCE_REPO}/pulls/${PR_NUM}" --jq '[.labels[].name] | join(",")'
into GITHUB_OUTPUT, then set PR_LABELS: ${{ steps.context.outputs.pr_labels }} on "Run fix agent" — in both reusable-fix.yml and the inline fix job at reusable-dispatch.yml:1205. This also returns current labels rather than an event-time snapshot and works for both trigger events. Mention extending the trimmed event_payload only as an alternative that requires the injection review the trimming comment demands. Since the docs point readers at "the wiring follow-up", file that follow-up issue with the corrected recipe and link it.
| # needs-human warning and the iteration summary reflect the cap pre-fix actually | ||
| # enforces. 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:-}")" |
There was a problem hiding this comment.
[MEDIUM] Budget-aware needs-human mirror in post-fix has zero test coverage
Verified at head cc462ec. The added hunk at lines 409-418 computes FIX_BUDGET="$(parse_fix_budget "${PR_LABELS:-}")", tightens BOT_CAP when the budget is lower, and derives WARN_THRESHOLD=$(( BOT_CAP - 1 )). That threshold drives both the needs-human label application (lines 425-430) and the "Iteration: N of M (bot cap)" summary line — i.e. it is the entire mechanism preventing the "reports 2 of 5, never escalates" regression the surrounding comment describes.
scripts/post-fix-test.sh (1032 lines, and itself untouched by this PR) contains zero occurrences of PR_LABELS, "budget", or "needs-human" — confirmed by grep -c on the head blob (0/0/0). The new pre-fix-test.sh tests cover only the parser and pre-fix enforcement; nothing exercises the post-fix mirror. Deleting the three-line tightening block would leave every existing test green while silently restoring the exact bug the PR set out to prevent, and this is a boundary-sensitive computation (WARN_THRESHOLD=0 at budget 1).
Provenance, so this does not read as a re-raise: the coverage ask appears only inside the collapsed "Agent Prompt" of the bot comment anchored on scripts/pre-fix.src.sh:118, whose headline claim ("Post-fix ignores effective budget") has since been fixed by this very hunk — so that thread reads as addressed. An earlier review body noted a fifth candidate was "dropped as a duplicate", but that is an unanchored review-body remark, not a posted finding, and three pushes later the coverage still does not exist. No inline thread on scripts/post-fix.src.sh or scripts/post-fix-test.sh exists at all.
Suggestion. Add post-fix-test.sh cases mirroring the pre-fix ones:
- (a)
TRIGGER_SOURCE=<bot>,FIX_ITERATION=2,ITERATION_CAP=5,PR_LABELS=fullsend-fix-budget/2assertsneeds-humanis applied and the summary reports "2 of 2", not "2 of 5"; - (b)
PR_LABELS=fullsend-fix-budget/9withITERATION_CAP=5has no effect (label can only tighten); - (c) budget=1 pins the
WARN_THRESHOLD=0boundary so iteration 1 escalates; - (d) a human trigger with a budget label still reports the full
ITERATION_CAP_HUMAN, matching the new comment at lines 441-444.
Malformed/absent labels should assert the unchanged global cap.
reusable-fix.yml is workflow_dispatch, so github.event.pull_request is empty. Resolve labels via the pulls API into GITHUB_OUTPUT. Cover the budget-aware needs-human path in post-fix-test. Signed-off-by: Benjamin Kapner <bkapner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
What
Adds a
fullsend-fix-budget/NPR label that 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. In
pre-fix, after the bot/human cap is selected, the parsed budget is applied asmin(label_budget, cap). Afullsend-fix-budget/2label on a PR that would otherwise get the bot cap of 5 stops the loop after 2 fix cycles; afullsend-fix-budget/99label is ignored (it cannot loosen the cap).Why
Today the fix-loop ceiling is global (
ITERATION_CAP/ITERATION_CAP_HUMAN). There is no per-PR knob when a maintainer wants a specific change to burn fewer cycles before escalating to a human, for example on a risky or expensive PR. A label is the lightest touch: it lives on the PR, needs no config change, and degrades safely.How
scripts/lib/fix-budget.lib.shwithparse_fix_budget, which extracts the smallest validfullsend-fix-budget/Nfrom a newline-separatedPR_LABELS. Malformed values (non-integer, zero, negative) are ignored rather than fatal, so a bad label never silently drops the existing cap.scripts/pre-fix.src.shsources the lib and applies the tightening after the cap is chosen (emits anoticewhen it takes effect).scripts/pre-fix-test.shunit-tests the parser directly (it is pure, so no forge mocks are needed) and is registered in the Makefilescript-testblock.scripts/pre-fix.shviamake script-build;make check-bundleandmake script-testpass.Scope note
PR_LABELSis a new optional input that the fix workflow does not populate yet. Wiring it (one line passing the PR's labels into the pre-fix step's env) is a natural follow-up; until then the feature is inert and the cap behaves exactly as before. Keeping the wiring separate keeps this PR to the parsing/enforcement logic plus its test.Relationship to the retro anti-retry-budget stance
The retro-analysis skill argues against retry budgets (
skills/retro-analysis/SKILL.md), but that is about masking test flakiness by retrying flaky tests, a correctness-signal concern. This is a different axis: a ceiling on how many times the review->fix loop runs before escalating to a human. It does not retry a failing check to make it pass; it bounds autonomous iteration. The two do not conflict.