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) DetailsScore 3 (elevated) preserved from prior assessment: Tier 1 signals are essentially unchanged (9 files, 387 lines in same 300-799 band, medium blast, 5 protected paths, test ratio 0.22), and Tier 2 remains very elevated with extreme fix/revert churn on post-triage.sh (55 all-time, 22 in 30d) and high multi-author contention (up to 10 distinct authors), partially offset by zero security sensitivity and no CI or dependency changes; composite 0.50x2.125 + 0.30x4.75 + 0.20x2.5 = 2.99 ~ 3. Previous runRisk 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 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 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 (4)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 (5)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). |
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)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 |
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 |
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>
0362b88 to
e48f683
Compare
|
🤖 Finished Review · ✅ Success · Started 10:18 AM UTC · Completed 10:38 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.94 |
| "recommended_fix": { "type": "string", "minLength": 1 }, | ||
| "proposed_test_case": { "type": "string", "minLength": 1 }, | ||
| "requires_workflow_changes": { "type": "boolean" } | ||
| "block_auto_promotion": { |
There was a problem hiding this comment.
[high] breaking-schema-change
The requires_workflow_changes boolean field is removed from triage_summary and replaced with block_auto_promotion. Because triage_summary declares additionalProperties: false, any triage agent output still containing requires_workflow_changes will be rejected by schema validation. The agent prompt, schema, and post-script are all updated in the same PR, so deployment is coordinated — but a race window exists during rolling deployments.
Suggested fix: Document in the PR description or release notes that this is an intentional flag-day change requiring atomic deployment. If staggered deployment is possible, add a transitional anyOf accepting either shape for one release cycle.
| **Auto-promotion blocked:** ${BLOCK_REASON}" | ||
| if [[ "${AUTO_CODE_ALLOWED}" == "true" ]]; then | ||
| echo "Applying triaged label (workflow changes required)..." | ||
| echo "::warning::Skipping ready-to-code — auto-promotion blocked (see comment for details)" |
There was a problem hiding this comment.
[medium] behavioral-contract-change
The GHA stdout warning string changed from ::warning::Triage detected workflow file changes required (#325) to ::warning::Skipping ready-to-code — auto-promotion blocked (see comment for details). The issue reference (#325) is removed.
Suggested fix: Document the warning string change in release notes if any external tooling matches the old string.
| "required": ["blocked", "reason"], | ||
| "properties": { | ||
| "blocked": { "type": "boolean" }, | ||
| "reason": { "type": "string", "minLength": 1, "maxLength": 1024 } |
There was a problem hiding this comment.
[medium] schema-constraint-tightening
block_auto_promotion.reason has minLength: 1 and is required even when blocked: false. A producer emitting {"blocked": false} without a reason will fail schema validation. The documented path for unblocked cases is to omit block_auto_promotion entirely. The test blocked-empty-reason-gets-fallback exercises a defense-in-depth path unreachable under normal schema validation.
Suggested fix: Add a comment to the test noting it exercises a defense-in-depth path. Consider whether reason should be optional when blocked: false.
|
|
||
| --- | ||
| **Auto-promotion blocked:** ${BLOCK_REASON}" | ||
| if [[ "${AUTO_CODE_ALLOWED}" == "true" ]]; then |
There was a problem hiding this comment.
[low] behavioral-change
The GHA ::warning:: annotation is only emitted when both BLOCKED=true and AUTO_CODE_ALLOWED=true. When auto-code is disabled and blocked, no warning appears on stdout. The block reason is still appended to the tracker comment for all categories.
| echo "PASS: ${test_name}" | ||
| } | ||
|
|
||
| run_test "comment-fenced-code-block-preserved" \ |
There was a problem hiding this comment.
[low] scope-creep
The test comment-fenced-code-block-preserved validates that fenced code blocks survive unchanged in the triage comment. This assertion does not trace to issue #2207 or to the block_auto_promotion work.
| # of ready-to-code only for categories that would otherwise auto-promote. | ||
| AUTO_PROMOTION_BLOCKED=false | ||
| if [[ "${BLOCKED}" == "true" ]]; then | ||
| if [[ -z "${BLOCK_REASON}" ]]; then |
There was a problem hiding this comment.
[low] error-handling-idiom
The empty-reason fallback ("No reason provided") handles a case the schema (minLength: 1) already prevents. This is defense-in-depth for unvalidated callers, but the intent is not documented.
Suggested fix: Add an inline comment such as: # Defence-in-depth: schema enforces minLength:1 but guard against unvalidated callers.
fullsend-ai-review
Bot
removed
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
[HIGH] Required check functional-tests (triage) is currently failing (eval max_turns regression)
Live CI on the current HEAD (e48f683) shows functional-tests (triage) and the downstream functional-tests-complete gate both FAILING (run: https://github.com/fullsend-ai/agents/actions/runs/34110624836/job/101705837083), confirmed via gh pr checks 1078. The PR's test-plan checklist only lists the two passing shell unit-test scripts; the required live-agent eval suite is red with REGRESSIONS: 1 detected / [max_turns] pass_rate: >= 1.0 -> 0.875 — one triage eval case now needs more turns than the baseline allows, most plausibly from the added open-ended judgment call in the new block_auto_promotion prompt clause. No existing PR comment addresses this CI status; the PR is not mergeable in its current state.
Suggestion: Inspect the eval-results-triage artifact from that run to find the regressed case, and either tighten the prompt so the extra reasoning step isn't needed, or deliberately update the eval baseline (called out explicitly in the PR description) if the extra turn is expected. Do not merge while this required check is red.
| COMMENT="${COMMENT} | ||
|
|
||
| --- | ||
| **Next steps:** This issue was held for review. Run \`/fs-code\` only after confirming the concerns above." |
There was a problem hiding this comment.
[MEDIUM] Held-for-review footer recommends /fs-code even when the block reason is the workflow-file permission limitation
When ACTION=="sufficient" and BLOCKED=="true", the appended footer unconditionally reads: "This issue was held for review. Run /fs-code only after confirming the concerns above." This text is identical regardless of why auto-promotion was blocked. For the one concrete gate this PR ships (CI/workflow-file changes), agents/triage.md itself explains the code agent cannot modify workflow files under current permissions — so inviting /fs-code "after confirming the concerns" is misleading for exactly the case the feature was built for; a human PR is what's actually needed, not a code-agent slash command. This is distinct from the earlier (now-resolved) thread on this same file about AUTO_PROMOTION_BLOCKED vs BLOCKED gating — which code path fires; this is about the wording of the footer that does fire, and no existing comment covers it.
Suggestion: Differentiate the footer text based on the blocking reason (or at minimum drop the unconditional /fs-code recommendation): for workflow/CI-permission blocks, point at a human PR instead of /fs-code; reserve "run /fs-code after confirming" for future non-permission blocking reasons.
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.The prompt's second block condition ("any other condition where auto-dispatch would be premature") is deliberately provisioned ahead of the effort-scoring gate (fullsend#2207 / follow-up PR) so reviewers and agents do not need to guess the intent. This PR only implements the workflow-file gate; the second clause is the hook for that follow-up.
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 wheneverblockedis true, including feature andTRIAGE_AUTO_CODE=off.Test plan
bash scripts/post-triage-test.shbash scripts/validate-output-schema-test.sh