feat(triage): generalize workflow-change block into block_auto_promotion - #1078
feat(triage): generalize workflow-change block into block_auto_promotion#1078rh-hemartin wants to merge 1 commit into
Conversation
PR Summary by QodoGeneralize triage auto-promotion blocking with reasons
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
|
🤖 Finished Review · ✅ Success · Started 11:45 AM UTC · Completed 12:06 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.18 |
Code Review by Qodo
1. triage.md adds agent directives
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Risk Assessment: elevated (3/5) DetailsElevated risk maintained at 3: 9 files with 361 changed lines and medium blast radius, 5 protected paths, test ratio 0.22. Very high fix/revert churn on post-triage.sh and multi-author contention keep Tier 2 elevated. Linked issue is well-scoped but Tier 2 churn signals sustain the elevated composite. Previous runRisk Assessment: elevated (3/5) DetailsElevated risk maintained at 3, consistent with prior assessment: 9 files with 504 changed lines and large blast radius across the triage subsystem, 5 protected paths modified, test ratio at 0.22. Tier 2 continues to show very high fix/revert churn and high commit frequency, confirming an actively unstable area. Linked issue is well-scoped (priority/medium feature) which provides mild downward pressure, but the sustained Tier 2 churn keeps the composite at elevated. Previous run (2)Risk Assessment: elevated (3/5) DetailsElevated risk maintained at 3, consistent with prior assessment: 9 files with 504 changed lines and large blast radius across the triage subsystem, 5 protected paths modified, test ratio at 0.22. Tier 2 continues to show very high fix/revert churn and high commit frequency, confirming an actively unstable area. Linked issue is well-scoped (priority/medium feature) which provides mild downward pressure, but the sustained Tier 2 churn keeps the composite at elevated. Previous run (3)Risk Assessment: elevated (3/5) DetailsElevated risk (up from prior moderate): 9 files with 504 changed lines and large blast radius across the triage subsystem, 5 protected paths modified, test ratio at 0.22, and very high fix/revert churn (3-40 per file in 90d) indicating an actively unstable area; score increased from 2 to 3 due to blast radius upgrading to large and sustained high churn in Tier 2. Previous run (4)Risk Assessment: moderate (2/5) DetailsModerate risk: 9 files with 445 changed lines across the triage subsystem, 5 protected paths modified, no CI/dependency changes, established contributor, but elevated churn in the triage scripts and docs (20-36 commits in 30 days). |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)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.
fa3c916 to
361fb43
Compare
|
🤖 Finished Review · ✅ Success · Started 12:42 PM UTC · Completed 1:02 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.68 |
fullsend-ai-review
Bot
added
risk/elevated
Superseded by updated review
fullsend-ai-review
Bot
added
the
requires-manual-review
361fb43 to
f09b3d7
Compare
|
🤖 Finished Review · ✅ Success · Started 9:04 AM UTC · Completed 9:46 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Five inline findings on scripts/post-triage.src.sh (1 high, 4 medium), all concerning the new fence stripper and the block_auto_promotion comment path.
f09b3d7 to
551a2af
Compare
|
🤖 Finished Review · ✅ Success · Started 5:31 AM UTC · Completed 5:51 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.60 |
Replace the single-purpose requires_workflow_changes boolean with a general-purpose block_auto_promotion object (blocked + reason). Workflow file detection now sets blocked=true with a reason; later gates can use the same field without a new post-script path. When blocked is true, the post-script appends the reason to the triage comment for every category. The label change (triaged instead of ready-to-code) still applies only to categories that would otherwise auto-promote. The reason is flattened for the comment (newlines and ANSI only) so :: and % stay intact for readers. Newlines cannot start a GHA workflow command on stdout. A held-for-review footer is used when auto-promotion is blocked. Signed-off-by: Hector Martinez <hemartin@redhat.com>
551a2af to
0362b88
Compare
|
🤖 Finished Review · ✅ Success · Started 8:05 AM UTC · Completed 8:27 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.98 |
|
|
||
| if [[ "${ACTION}" == "sufficient" ]]; then | ||
| COMMENT="${COMMENT} | ||
| if [[ "${AUTO_PROMOTION_BLOCKED}" == "true" ]]; then |
There was a problem hiding this comment.
[HIGH] Held-for-review footer still keys off AUTO_PROMOTION_BLOCKED, so it doesn't fire for feature/off-auto-code blocked cases despite the body now saying 'Auto-promotion blocked'
Lines 448-461: when BLOCKED=="true" the reason is now appended to COMMENT unconditionally (fixed per the resolved thread at old line 494, where the maintainer noted 'Appending always regardless of AUTO_CODE_ALLOWED'), but AUTO_PROMOTION_BLOCKED is still only set to true inside the nested if [[ "${AUTO_CODE_ALLOWED}" == "true" ]] branch. The footer logic here branches on AUTO_PROMOTION_BLOCKED, not BLOCKED: if [[ "${AUTO_PROMOTION_BLOCKED}" == "true" ]] prints the held-for-review footer, else it prints the normal '/fs-code — agent creates a PR to implement this issue' footer. So for category=feature (never in TRIAGE_AUTO_CODE_CATEGORIES by default, so AUTO_CODE_ALLOWED stays false) or TRIAGE_AUTO_CODE=off with any category, a blocked issue's comment body says 'Auto-promotion blocked: ' and is immediately followed by the standard footer inviting /fs-code — a self-contradicting comment on the tracker. The PR description states as fact: 'A held-for-review footer is used when auto-promotion is blocked' — true only for the subset of categories that would otherwise auto-promote. Confirmed untested: the existing blocked-feature-block-reason-in-comment test (post-triage-test.sh) only asserts the 'Auto-promotion blocked:' substring is present, never what footer follows; the only footer-content test (blocked-held-for-review-footer) uses category=bug, which does get AUTO_PROMOTION_BLOCKED=true and so never exercises the broken path. This is the same substance flagged in the suggestion text of the now-resolved review thread at scripts/post-triage.src.sh:494 ('...use the held-for-review footer instead of the /fs-code invitation in the TRIAGE_AUTO_CODE=off case'), but that thread's fix only addressed the body-append half of the suggestion — the footer half was never fixed, and the bug lives on a different code path over 250 lines away.
Suggestion: Drive the held-for-review footer off BLOCKED=="true" directly (or a dedicated 'reason was appended to the comment' flag set unconditionally alongside the body append), not off AUTO_PROMOTION_BLOCKED/AUTO_CODE_ALLOWED. Add a footer-content assertion (not just body-substring) to the feature-blocked and TRIAGE_AUTO_CODE=off test cases to lock in the fix.
| # Auto-promotion gate (#325): the triage agent can block auto-promotion | ||
| # via block_auto_promotion.blocked (e.g., workflow file changes). When | ||
| # blocked, the reason is appended to the comment. Bug/docs/performance | ||
| # categories receive triaged instead of ready-to-code. tostring is used |
There was a problem hiding this comment.
[MEDIUM] Code comment misstates jq's // semantics — tostring adds nothing here
The comment reads: 'tostring is used so an explicit blocked:false is not treated as missing (jq's // would).' Empirically disproved with jq 1.7: echo '{"blocked": false}' | jq -r '.blocked // false' and the same piped through | tostring both print false; jq's // only substitutes on null/false-y absent values in the sense of jq truthiness, and -r already prints booleans unquoted, so tostring changes nothing here (also confirmed for blocked:true and the missing-field case). No functional impact — the runtime check at [[ "${BLOCKED}" == "true" ]] behaves identically either way — but the comment states an incorrect tool-behavior claim as fact and will mislead a future maintainer editing this line (e.g. into thinking tostring is load-bearing when it is not, or removing it for the wrong reason).
Suggestion: Correct or remove the comment. If tostring is kept defensively (e.g. in case schema validation is bypassed and a non-boolean value slips through), say that explicitly instead of citing the incorrect // semantic.
| # stdout (_gha_sanitize). An embedded newline is flattened so it cannot | ||
| # start a workflow command if a later stdout echo is added. | ||
| _comment_sanitize() { | ||
| printf '%s' "$1" | tr -d '\n\r' | sed 's/\x1b\[[0-9;]*[a-zA-Z]//g' |
There was a problem hiding this comment.
[MEDIUM] _comment_sanitize deletes embedded newlines instead of replacing them with a separator, garbling multi-sentence block reasons
_comment_sanitize() { printf '%s' "$1" | tr -d '\n\r' | sed 's/\x1b\[[0-9;]*[a-zA-Z]//g'; } is applied to BLOCK_REASON before it's appended into the tracker comment body. tr -d '\n\r' deletes newlines outright rather than replacing them with whitespace, so an agent-authored reason like "Line 1.\nLine 2." is posted as "Line 1.Line 2." with words run together and no separator — the schema (schemas/triage-result.schema.json) allows arbitrary strings up to 1024 chars in this field, including newlines, and nothing upstream forbids multi-line reasons. This is a new, distinct issue from the earlier resolved thread about percent-encoding on this code path (which concerned _gha_sanitize, not the newline-deletion behavior of the newer _comment_sanitize); no existing PR comment addresses the missing separator.
Suggestion: Replace newlines with a space instead of deleting them, e.g. tr '\n\r' ' ', which keeps the defense-in-depth intent (no embedded newline that could start a workflow command on a later stdout echo) while keeping multi-line reasons readable. Markdown would collapse a literal newline into a soft-wrap space anyway, so this is strictly safer with no loss.
| **Workflow change detection (optional):** If the issue likely requires modifying CI/pipeline configuration files (`.github/workflows/`, `.gitlab-ci.yml`, `.fullsend/.github/workflows/`, or enrolled-repo shim workflows), set `requires_workflow_changes: true` in `triage_summary`. When set, the post-triage script skips auto-triggering the code agent because the code agent cannot modify workflow files under current permissions. The triage comment should warn about this limitation and note that manual intervention is required. When `requires_workflow_changes` is not set or is `false`, auto-triggering proceeds normally. | ||
| **Blocking auto-promotion:** Use the `block_auto_promotion` field in `triage_summary` to prevent the post-triage script from auto-promoting the issue to the code agent. Set `blocked: true` with a `reason` when: | ||
| - The fix requires modifying CI/pipeline configuration files (`.github/workflows/`, `.gitlab-ci.yml`, `.fullsend/.github/workflows/`, or enrolled-repo shim workflows) that the code agent cannot modify under current permissions. | ||
| - Any other condition where auto-dispatch would be premature. |
There was a problem hiding this comment.
[MEDIUM] Triage prompt's block condition includes an unbounded 'any other condition' clause broader than the one concrete gate this PR ships
The prompt instructs the agent to set blocked: true when '(1) The fix requires modifying CI/pipeline configuration files...' OR '(2) Any other condition where auto-dispatch would be premature.' Clause (2) is open-ended and, unlike clause (1), maps to no concrete gate implemented in this PR — the PR description frames the generalization as groundwork for a later effort-scoring gate (fullsend#2207/#1079), not as a currently-defined second condition. There is no test exercising this open-ended clause, and no evaluation data cited constraining what the agent will treat as 'premature.' This is not a functional bug — it's a scope/precision gap: shipping an unbounded instruction ahead of the concrete gate that would justify it risks the agent over-applying blocked:true (regressing auto-promotion for bug/documentation/performance) in ways this PR's test suite cannot catch, since the test suite only exercises the concrete workflow-file case.
Suggestion: Scope the instruction to the concrete workflow-file case for this PR and introduce the broader 'any other premature condition' clause in the follow-up PR (#1079) once there is a second concrete gate to anchor it to, or keep it but note in the PR description that it's deliberately provisioned ahead of the effort-scoring gate so reviewers/agents don't need to guess the intent.
2 participants
Summary
Replace the single-purpose
requires_workflow_changesboolean with a general-purposeblock_auto_promotionobject (blocked+reason). Workflow-file detection now uses that field so later gates (effort scoring) can reuse the same post-script path and attach a reason.This is the first of two slices split from
feat/2207-effort-gating. Effort estimation is not in this PR.Ref fullsend-ai/fullsend#2207
Behavior
blockedis true, the post-script appends**Auto-promotion blocked:** <reason>to the triage comment for every category.blocked=truealso appliestriagedinstead ofready-to-code. Categories that already go totriaged(feature, security, other) keep that label.requires_workflow_changesis removed; the schema and post-script only acceptblock_auto_promotion.::and%stay intact. A held-for-review footer is used when auto-promotion is blocked.Test plan
bash scripts/post-triage-test.shbash scripts/validate-output-schema-test.sh