feat(triage): effort-estimation skill and auto-promotion gate - #646
feat(triage): effort-estimation skill and auto-promotion gate#646rh-hemartin wants to merge 1 commit into
Conversation
|
🤖 Finished Review · ✅ Success · Started 1:05 PM UTC · Completed 1:23 PM UTC |
PR Summary by QodoAdd effort-estimation gating and block_auto_promotion to triage auto-promotion
AI Description
Diagram
High-Level Assessment
Files changed (30)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsHigh
Medium
Low
Labels: PR modifies triage agent instructions, post-script, skill, and eval cases. 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.
b672253 to
3d72bb1
Compare
|
🤖 Review · |
3d72bb1 to
2686001
Compare
|
🤖 Review · |
2686001 to
743fabd
Compare
|
🤖 Review · |
743fabd to
f8405d6
Compare
|
🤖 Finished Review · ✅ Success · Started 3:00 PM UTC · Completed 3:16 PM UTC |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
[MEDIUM] Effort-estimation threshold and traceability are unverified before merge (PR description / skills/effort-estimation/SKILL.md)
Three compounding gaps:
-
The PR body says "Closes #2207", but
repos/fullsend-ai/agents/issues/2207returns 404 — issue #2207 does not exist in this repo. The actual issue isfullsend-ai/fullsend#2207("dispatch: no effort-based gating before auto-promoting issues to coder"), which is open. GitHub's auto-close syntax only closes same-repo issues unless theowner/repo#numberform is used, so merging as-is will not close the tracked issue, and the entire feature (the four scoring dimensions, 1-5 scale, and the specific ">=4 blocks" threshold) is only traceable to a cross-repo issue that the PR text doesn't correctly reference. -
There is no cited derivation (historical data, prior incidents) for the ">=4 average blocks" threshold in
skills/effort-estimation/SKILL.md; it reads as a picked-because-it-feels-right number, and no eval case near the boundary (e.g. ~3.5-4.5) exists to confirm the cutoff behaves as intended — the two added fixtures (eval/triage/cases/008, 009) are deliberately constructed extremes. -
The PR's own test plan leaves unchecked: "Eval run confirms case 008 gets
triagedand 009 getsready-to-code" — meaning the one piece of evidence that would validate the threshold against a real triage agent run has not been collected, and this is shipped as final production gating logic rather than as an explicitly-flagged, tunable heuristic pending that verification.
Suggestion: update the PR description to "Closes fullsend-ai/fullsend#2207" so the cross-repo issue actually closes on merge. Before merging the gating logic as final, run the eval harness on cases 008/009 (and ideally a boundary case around effort ~4.0) and record actual pass/fail, or explicitly mark the threshold as a tunable default subject to recalibration pending that evidence.
|
About the compounding gaps: Fixed the PR body, the decision of using 1 to 5 has been raised in internal communications, not sure there is a reference and I don't think we need a middle case, it will bring flakyness, let's see how this goes; and the test plan has been checked. |
Add an effort-estimation skill that scores issues on four dimensions
(1-5 each). An average >= 4 blocks auto-promotion, keeping the issue
at triaged for human review.
Replace requires_workflow_changes with block_auto_promotion ({blocked,
reason}) in triage_summary and the post-script gate. Harden the
post-script against workflow-command injection.
Add eval cases 008 and 009 with runnable fixture repos.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Hector Martinez <hemartin@redhat.com>
f8405d6 to
6423d80
Compare
|
🤖 Finished Review · ✅ Success · Started 6:32 AM UTC · Completed 6:51 AM UTC |
|
|
||
| if echo "${COMMENT}" | grep -q '```'; then | ||
| echo "::warning::Stripping fenced code blocks from triage comment" | ||
| COMMENT=$(echo "${COMMENT}" | sed '/^```/d') |
There was a problem hiding this comment.
[medium] logic-error
The fenced code block stripping uses sed '/^```/d' which only deletes the fence delimiter lines but preserves all code content lines between them. After stripping, raw code appears as unformatted prose in the posted GitHub comment. The test comment claims this 'mirrors post-scribe.sh enforcement,' but post-scribe.sh uses gate_reject to reject the entire item when code blocks are detected - it does not attempt partial stripping.
Suggested fix: Replace sed '/^/d' with sed '/^/,/^```/d' to strip the full fenced code block including its content, or update the warning message to accurately describe the behavior.
| BLOCK_REASON=$(jq -r '.triage_summary.block_auto_promotion.reason // empty' "${RESULT_FILE}") | ||
| # Collapse runs of 2+ colons to a single colon so untrusted text | ||
| # can never form a GHA workflow command (e.g., ::error::). | ||
| while [[ "${BLOCK_REASON}" == *::* ]]; do |
There was a problem hiding this comment.
[low] injection-vuln
The prior finding (triple-colon bypass of :: sanitization) is now fixed. The new while loop iterates until no :: sequences remain, correctly collapsing :::error::: to :error:. BLOCK_REASON never reaches the GHA log - it is only embedded in COMMENT via printf. Residual risk is negligible.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass (inline comments added below). One additional finding on unchanged code that isn't part of this PR's diff, so noting it here instead of inline:
[HIGH] Auto-promotion gate is undermined by the unconditional /fs-code footer appended to the same comment (scripts/post-triage.sh, the "Append action-hints footer (sufficient only)" block, ~line 558)
When block_auto_promotion.blocked is true and category is bug/documentation/performance, the script appends "Auto-promotion blocked: <reason>". But the unconditional "Next steps" footer, gated only on ACTION == "sufficient" with no check of AUTO_PROMOTION_BLOCKED, still appends "/fs-code — agent creates a PR to implement this issue". docs/code.md confirms /fs-code triggers the code agent independent of label state ("triggered when the ready-to-code label is applied... or via the /fs-code command"). The result is a single comment that both explains why the issue was held for review and hands the reader a working one-click bypass of that exact hold.
Suggested fix: Suppress or reword the Next-steps footer when AUTO_PROMOTION_BLOCKED is true, e.g. "This issue was held for review — run /fs-code only after confirming the concerns above."
| "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] Schema does not require block_auto_promotion for the categories the gate is meant to cover
block_auto_promotion is entirely optional inside $defs.triage_summary (not in the required array a few lines up), and unlike the top-level allOf block elsewhere in this schema which conditionally requires fields based on action, there is no conditional requiring block_auto_promotion when category is bug/documentation/performance. scripts/post-triage.sh reads it as jq -r '.triage_summary.block_auto_promotion.blocked // false', so if the model omits the field entirely (prompt drift, rushed run, or issue-content injection), the omission is indistinguishable from an explicit blocked: false and the issue silently auto-promotes to ready-to-code with no schema-validation error and no warning — exactly the scenario the gate exists to prevent. This is distinct from the existing "edge-case" thread on this object, which only discusses blocked/reason being required within the object once present, not the object's own presence being unenforced for the categories the gate targets.
Suggested fix: Add a conditional requirement (mirroring the existing top-level allOf pattern) inside $defs.triage_summary: when category is bug/documentation/performance, require block_auto_promotion to be present. This turns a silent gate bypass into a hard schema-validation error.
| @@ -0,0 +1,83 @@ | |||
| --- | |||
There was a problem hiding this comment.
[MEDIUM] New skill introduces a second, unreconciled "Effort" scoring scheme instead of reusing/aligning with the prioritize agent's existing RICE Effort dimension
fullsend-ai/fullsend#2207 (the issue this PR closes) explicitly names existing infrastructure to build on — both the issue body ("The prioritize agent already computes an Effort dimension via RICE") and the prioritize agent's own RICE comment on that issue ("Effort: 1 — The RICE prioritize agent already computes an Effort dimension, so the core estimation logic exists"). Instead, this skill defines an independent 4-dimension (scope/testing/domain/risk), 1-5, averaged rubric with a hardcoded >=4 block threshold. agents/prioritize.md shows the existing RICE Effort dimension uses a 0.25-3 scale with inverse semantics (higher effort = lower priority, used as a denominator). Neither the PR body, docs/triage.md, nor this file explains why the existing dimension wasn't reused or how the two same-named "Effort" concepts (different scales, different semantics, no cross-reference) are meant to coexist.
Suggested fix: Add a short note explaining why an independent scale was introduced (e.g., prioritize runs on a schedule, decoupled from synchronous triage), or align this rubric's scale/semantics with the prioritize agent's Effort definition.
| 4. High regression risk, needs careful rollout | ||
| 5. Breaking change affecting downstream consumers | ||
|
|
||
| Compute the overall effort as the average of the four dimensions, rounded |
There was a problem hiding this comment.
[MEDIUM] Averaging the four effort dimensions lets a single maximal-risk dimension slip through the gate undetected
Step 3 blocks only when the straight average of scope/testing/domain/risk is >= 4. An issue scoring Scope=1, Testing=1, Domain=1 but Risk=5 ("Breaking change affecting downstream consumers", the top of the Risk rubric above) averages to 2.0 and is NOT blocked — a change the rubric itself flags as breaking for downstream consumers would still auto-promote unattended. Both new eval fixtures (008 and 009) score uniformly across all four dimensions, so this dilution failure mode — the exact scenario the gate exists to catch — is completely untested by this PR's own eval suite.
Suggested fix: Use max() or a rule that any single dimension >= 4 forces blocking, rather than a straight average. Add an eval case with a mixed profile (one dimension at 5, others at 1) to confirm intended blocking behavior.
| auth middleware, rate_limit.py) — 4. | ||
| - Testing: no existing session lifecycle tests; needs new test | ||
| infrastructure for time-dependent behavior (mocking time.time) — 4. | ||
| - Domain knowledge: requires understanding session token security |
There was a problem hiding this comment.
[MEDIUM] Domain-knowledge annotation score contradicts the skill's own rubric, making this fixture potentially flaky at the >=4 threshold
This annotation scores "Domain knowledge... coordinating invalidation across the session store, auth middleware, and rate limiter" as a 4. But skills/effort-estimation/SKILL.md defines domain-knowledge level 4 as "Requires cross-repo or external API knowledge" and level 3 as "Requires understanding subsystem design". The described scenario (session store + auth middleware + rate limiter, all confirmed single-repo/in-process components per this fixture's own description) matches level 3, not 4. Recomputing with domain=3 instead of 4 (scope=4, testing=4, domain=3, risk=4) gives (4+4+3+4)/4 = 3.75, rounding to 3.8 per the skill's "rounded to one decimal place" rule — below the >=4 blocking threshold. A model faithfully applying the documented rubric could legitimately set blocked: false, contradicting this fixture's required triaged label and forbidden ready-to-code label.
Suggested fix: Either revise the justification to genuinely require cross-repo/external-API knowledge (matching level 4), or lower the expected score to 3 and re-verify the threshold math with the other three dimensions still yields >=4.
| ``` | ||
|
|
||
| **Workflow change detection (optional):** If the issue likely requires modifying GitHub Actions workflow files (`.github/workflows/`, `.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: |
There was a problem hiding this comment.
[MEDIUM] "Blocking auto-promotion" section overstates block_auto_promotion's effect versus what post-triage.sh actually implements
This line tells the model it can set blocked: true for "any other condition where auto-dispatch would be premature" (not just effort/workflow-changes) and states unqualified that "the post-script applies triaged instead of ready-to-code and appends the reason to the triage comment." In reality, scripts/post-triage.sh only honors block_auto_promotion inside a gate requiring CATEGORY to be bug/documentation/performance — for feature, security, or other categories the flag is silently ignored and the "Auto-promotion blocked:" footer is never appended. A maintainer or future contributor reading only this section would reasonably expect the reason to always surface when blocked: true, which is false for three of six category values.
Suggested fix: Qualify the doc to state explicitly that the reason-append/triaged-override behavior only applies to bug/documentation/performance categories, matching the CATEGORY guard in post-triage.sh.
2 participants
Summary
effort-estimationskill that scores implementation effort on a 1-5 scale across scope, testing, domain knowledge, and risk. Issues scoring >= 4 block auto-promotion.requires_workflow_changesboolean withblock_auto_promotionobject ({blocked, reason}) intriage_summary. The post-script gatesready-to-codevstriagedfor bug/documentation/performance categories.::warning::uses a fixed message (no interpolation of untrusted content), andBLOCK_REASONis sanitized (::collapsed to:) before any use.commentfield to prose-only (no fenced code blocks) to avoid model abbreviation of identifiers inside JSON strings.Migration: the schema replaces
requires_workflow_changeswithblock_auto_promotion. Becausetriage_summaryusesadditionalProperties: false, all components (schema, prompt, post-script) must be deployed and rolled back together per the lockstep release model.Closes fullsend-ai/fullsend#2207
Test plan
pytest+ server endpoints)triagedand 009 getsready-to-code🤖 Generated with Claude Code