feat(triage): add effort-based gating via effort-estimation skill#36
feat(triage): add effort-based gating via effort-estimation skill#36rh-hemartin wants to merge 1 commit into
Conversation
|
🤖 Review · |
PR Summary by QodoAdd effort scoring and auto-promotion gating to triage pipeline
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Protected scripts/ files modified
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
🤖 Finished Review · ✅ Success · Started 6:35 AM UTC · Completed 6:44 AM UTC |
ReviewFindingsMedium
Low
Labels: PR modifies triage agent prompt, post-script, and adds effort-estimation skill to the triage pipeline. Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewVerdict: comment · 2 medium, 2 low findings The new head (09a16a7) is a force-push from the previously-reviewed 8408ffc. The key code change: Three prior low findings are resolved:
The effort-gating logic is correctly implemented: FindingsMedium
Low
Previous run (4)ReviewVerdict: approve · 0 medium, 5 low findings The new head (8408ffc) is a force-push from b2b78ae that addresses all three prior medium findings:
The effort-gating logic is correctly implemented: Findings1. No test for effort just below threshold · low · test-adequacyFile: The test suite covers effort=0.25, 1.0 (below threshold) and effort=2.0, 2.5, 3.0 (at/above threshold), but no test verifies effort=1.5 or 1.99 routes to Remediation: Add a test case with effort=1.5 or 1.99 asserting 2. Test fixtures for non-applicable categories include effort values · low · test-integrityFile: The Remediation: Remove effort from feature/other fixtures, or add a comment explaining they verify effort is ignored for non-applicable categories. 3. Schema validation tests don't cover conditional effort requirement · low · test-adequacyFile: The schema validation test suite has one Remediation: Add schema validation test cases: a bug category without effort that fails, and a feature category without effort that passes. 4. No linked issue for non-trivial feature change · low · authorizationFile: The PR references #2207 in a code comment but no issue URL is linked in the PR metadata. The PR body states it was "Migrated from fullsend-ai/fullsend feat/triage-effort-analysis", providing implicit authorization. 5. Triage effort estimation overlaps with prioritize agent's RICE effort · low · architectural-coherenceFile: The triage agent now estimates effort (0.25–3 scale) matching the prioritize agent's RICE effort dimension. Triage effort gates auto-promotion (< 2.0 →
Previous run (5)ReviewVerdict: comment · 3 medium, 5 low findings The new head (b2b78ae) is a force-push from the previously-reviewed 8e5ff3f. The effort-gating code across all four files is functionally identical to the prior review. All eight prior findings remain unaddressed. The effort-gating logic is correctly implemented: Findings1. GHA workflow command injection via unsanitized EFFORT interpolation · medium · securityFile: Two interpolation points expose unsanitized
Note: This is defense-in-depth — schema validation ( Remediation: Sanitize EFFORT before all SAFE_EFFORT="${EFFORT//::/}"
SAFE_EFFORT="${SAFE_EFFORT//%0A/}"
SAFE_EFFORT="${SAFE_EFFORT//%0a/}"
SAFE_EFFORT="${SAFE_EFFORT//%0D/}"
SAFE_EFFORT="${SAFE_EFFORT//%0d/}"2. Documentation now contradicts actual behavior —
|
fullsend-ai-review
Bot
added
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review squad (5 agents: Claude, Gemini, Codex) findings — Medium and above, deduplicated and verified against the head branch. Two additional Medium findings below couldn't be attached inline since they target files/state outside this PR's diff:
[MEDIUM] docs/triage.md control-label documentation is now stale — docs/triage.md:40-41 (not touched by this PR) still describes ready-to-code as "low-risk (bug, documentation, performance)" and triaged as "feature or other category," with no mention of the new effort threshold. docs/code.md:41 has the same staleness. Already flagged independently by the qodo-code-review bot on this PR ("Docs contradict effort gating").
[MEDIUM] Branch is 24 commits behind main; CI test suite has never run against it — git compare main...feat/triage-effort-analysis shows the branch diverged and is missing main's 88800b2 (adds a label-contradiction guard to the same sufficient) case block this PR modifies) plus the Makefile/script-test.yml CI infra added after this branch was cut. No script-test check has run on this PR — only dispatch/dispatch and DCO. Suggest rebasing onto current main before merge and confirming the effort-gating logic composes cleanly with the label-contradiction guard.
Four inline comments above cover the remaining High/Medium findings (stale triaged label bug, effort-scale mismatch with prioritize.md, optional-but-load-bearing effort schema field, and the round-down bias prompt-injection surface).
c51787d to
24b4069
Compare
|
🤖 Finished Review · ✅ Success · Started 6:26 PM UTC · Completed 6:35 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
24b4069 to
2856f9d
Compare
|
🤖 Finished Review · ✅ Success · Started 6:38 PM UTC · Completed 6:51 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
2856f9d to
60d8500
Compare
|
🤖 Finished Review · ✅ Success · Started 7:13 PM UTC · Completed 7:28 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Review · |
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 8:02 PM UTC · Completed 8:11 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
213c3b9 to
39c1f2a
Compare
|
🤖 Review · |
b2b78ae to
8408ffc
Compare
|
🤖 Finished Review · ✅ Success · Started 8:44 AM UTC · Completed 8:59 AM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review squad (4 agents: Claude, Gemini, Codex) — new findings only, deduplicated against the 8 existing review threads.
[HIGH] Two label-category test fixtures missing effort field silently exercise wrong routing path (scripts/post-triage-test.sh:477-484)
The label-category-contradiction-stripped (category: documentation) and label-category-consistent-passes (category: bug) fixtures were not updated with the effort field. After this PR, missing effort for bug/docs/performance routes to triaged instead of ready-to-code. Both tests still pass because their assertions check label_actions side-effects (warning messages, area labels) rather than the routing label — but they now silently exercise the missing-effort fallback instead of the normal low-effort path.
Suggested fix: add "effort":1.0 to both fixtures, consistent with every other sufficient/bug fixture updated in this PR.
Three additional MEDIUM findings posted inline (schema then clause fragility, no negative schema validation test, effort=2.0 boundary test gap).
8408ffc to
137354b
Compare
|
🤖 Review · |
137354b to
2e5d15d
Compare
|
🤖 Review · |
2e5d15d to
09a16a7
Compare
|
🤖 Finished Review · ✅ Success · Started 8:15 AM UTC · Completed 8:29 AM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
09a16a7 to
39a75aa
Compare
|
🤖 Finished Review · ✅ Success · Started 6:47 AM UTC · Completed 7:05 AM UTC |
![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.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review squad pass (models: Claude, Grok), deduplicated against all prior review threads and verified against head 39a75aa. All eight July 15 fixes are confirmed correctly in place, and both test suites pass at head. Posting 3 new MEDIUM findings inline; a 4th MEDIUM is an escalation of the existing unreplied July 20 performance-label thread and was posted as a reply there to keep the discussion in one place.
39a75aa to
350f215
Compare
|
🤖 Review · |
350f215 to
3ca1484
Compare
|
🤖 Finished Review · ✅ Success · Started 10:31 AM UTC · Completed 10:48 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review squad pass (models: Claude, Grok) at head 3ca1484, deduplicated against all prior review threads. All 11 previously fixed findings are confirmed in place, and both test suites pass at head (76 tests). Posting 3 new MEDIUM findings inline. The three still-unreplied threads (silent performance label auto-creation, docs/triage.md staleness, dual effort-estimation systems) were independently rediscovered by this round's agents but are not re-posted here — see their existing threads.
| "proposed_test_case": "Conceptual description of a test that would verify the fix — what to test, expected vs actual behavior, and edge cases to cover. Do not assume a specific test framework or file layout.", | ||
| "effort": 1.0 | ||
| }, | ||
| "comment": "A triage summary comment formatted in markdown, presenting the assessment to the maintainers. Include the proposed test case as a fenced code block.", |
There was a problem hiding this comment.
[medium] human-review-visibility
The entire purpose of the effort gate is to route high-effort issues to a human, but nothing surfaces the effort estimate or the gating reason to that human. The score lives only in agent-result.json, and the "High effort (N) — deferring triaged label" message goes to the Actions log. This comment field guidance was not updated, so a maintainer sees triaged + bug on a fully-specified bug with no visible explanation of why the coder was not dispatched — indistinguishable from an other-category triage.
Suggested fix: Either extend this field's description to require including the effort estimate (and, for effort >= 2.0, a one-line note that the issue was held for human review because of estimated effort), or have post-triage.sh deterministically append a standard line in the high-effort branch, e.g. "Effort estimated at 2.5/3 — held for human review; apply ready-to-code to dispatch." The deterministic option keeps the disclosure out of the LLM's hands.
Flagged independently by 2 of 3 agents this round.
| "simplest possible fix," "no architectural changes." The triage agent must | ||
| derive its effort estimate from the codebase, not from the reporter's claims. | ||
|
|
||
| What inspecting the code reveals (effort >= 2.0): |
There was a problem hiding this comment.
[medium] premature-decision / eval-consistency
This case's effort oracle sits in unresolved tension with case 001 on the same python-webapp fixture. 007 asserts any proper fix is effort >= 2.0, citing the codebase-wide absence of an error-handling pattern as "architectural work that affects every future endpoint" — but 001 uses the same fixture, its triage_expectations point 4 explicitly cites the same missing error handling as "a real finding," and still requires ready-to-code (i.e. effort < 2.0). An agent consistently applying 007's rubric can plausibly score 001 at >= 2.0 and fail its required-label assertion; with min_pass_rate: 0.9 over 7 cases, a single label miss fails the suite. By the PR's own rubric, "1.5 = Moderate — multiple components or some design work" is also a defensible reading of this fixture, so the >= 2.0 oracle rewards scope inflation.
Separately, the judge rubric's score-3 outcome ("identifies the bug correctly but doesn't push back on the effort characterization") necessarily produces ready-to-code, which the hard forbidden check fails independently — that rubric midpoint can never coexist with a passing case.
Suggested fix: Add explicit effort guidance to 001's annotations distinguishing the minimal fix from 007's reading (e.g. "the minimal correct fix is URL-decoding handling, effort < 2.0; noting broader error-handling gaps should not inflate the estimate"), consider strengthening this fixture so >= 2.0 is unambiguous under the rubric, and run the triage eval suite against the new prompt before merge — this case was added in the latest push and has not yet answered its own oracle.
Flagged independently by 2 of 3 agents this round.
| echo "::warning::Non-numeric effort value '${EFFORT_SAFE}' — treating as high effort for human review" | ||
| remove_label "ready-to-code" | ||
| DEFERRED_LABEL="triaged" | ||
| elif awk -v effort="${EFFORT}" 'BEGIN { exit (effort >= 2.0 ? 0 : 1) }'; then |
There was a problem hiding this comment.
[medium] premature-decision / configurability
The 2.0 cutoff between auto-dispatch and human review is a hardcoded literal applied uniformly to every consumer repo of this shared pipeline, with no env var or harness knob to tune it and no calibration data justifying 2.0 over 1.5 or 2.5. If the threshold turns out miscalibrated for a given repo (too much routed to human review, or too much auto-dispatched), the only recourse is patching this shared script.
Suggested fix: Expose the threshold as an env-configurable value read by this script (defaulting to 2.0, settable per-repo via the harness env block like other triage settings), or explicitly note in docs/triage.md that 2.0 is a starting heuristic subject to revision once real triage data exists. Reasonable as a v1 either way — flagging so the hardcoding is a recorded decision rather than an accident.
Minor related asymmetry on this exact line: any awk failure (non-zero exit for reasons other than the comparison, e.g. missing binary) is indistinguishable from "effort < 2.0" and falls through to auto-promotion — the only failure mode in this block that dispatches the coder instead of routing to a human. Inverting the test polarity (awk success = promote) would make infrastructure failure fail safe like the missing/non-numeric branches.
Introduce a new skill that users can override to control how effort is scored and when issues require human review before auto-promoting to the coder agent. The skill scores implementation effort on a 0.25-3 scale and returns an effort_requires_review boolean that the post-script uses to route issues to human review or auto-dispatch. The skill owns the scoring table, signals, threshold, and estimation rules. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
3ca1484 to
0829644
Compare
|
🤖 Finished Review · ✅ Success · Started 6:55 AM UTC · Completed 7:10 AM UTC |
rh-hemartin
changed the title
| @@ -59,7 +59,7 @@ run_test "valid-insufficient" \ | |||
| "true" | |||
|
|
|||
| run_test "valid-sufficient" \ | |||
There was a problem hiding this comment.
[medium] test-integrity
The valid-sufficient test fixture has category bug with effort: 1.0 but is missing effort_requires_review. The new schema conditional (allOf entry) requires both effort and effort_requires_review when category is bug/documentation/performance. This test will fail against the PR head schema.
Suggested fix: Add "effort_requires_review":false to the valid-sufficient test fixture's triage_summary.
| | `needs-info` | The issue lacks sufficient information. The agent posted clarifying questions. | | ||
| | `ready-to-code` | The issue is fully specified and low-risk (bug, documentation, performance). Bug and documentation categories also receive their eponymous labels (`bug`, `documentation`) automatically. Triggers the [code agent](code.md). | | ||
| | `triaged` | The issue is fully specified but is a feature or other category that requires human prioritization before coding. | | ||
| | `ready-to-code` | The issue is fully specified and low-risk (bug, documentation, performance) with effort below the review threshold. Bug and documentation categories also receive their eponymous labels (`bug`, `documentation`) automatically. Triggers the [code agent](code.md). | |
There was a problem hiding this comment.
[medium] docs-currency
Three documentation gaps: (1) ready-to-code description mentions bug and documentation eponymous labels but not performance; (2) bug and documentation rows say 'Applied alongside ready-to-code' but high-effort issues receive triaged instead; (3) no performance row in the control labels table despite performance being applied as a category label.
Suggested fix: Update ready-to-code to mention performance. Add a performance row. Update bug and documentation descriptions to reference both ready-to-code and triaged.
| "proposed_test_case": { "type": "string", "minLength": 1 } | ||
| "proposed_test_case": { "type": "string", "minLength": 1 }, | ||
| "effort": { | ||
| "type": "number", |
There was a problem hiding this comment.
[low] schema-description
The effort field description says '(0.25 = trivial, 1 = moderate, 2 = substantial, 3 = large)' but skills/effort-estimation/SKILL.md uses different labels: 0.25=Trivial, 1=Medium, 1.5=Moderate, 2=Complex, 3=Very complex. The schema also omits the 0.5 and 1.5 scores.
Suggested fix: Update the schema description to match the SKILL.md scoring table.
| @@ -271,6 +272,8 @@ Information is sufficient for a developer to investigate and fix. | |||
|
|
|||
| **Choosing a category:** the `feature` category covers issues that describe desired new behavior rather than a defect in existing functionality — the reporter expects something that has never been implemented. Use `feature` only when the described behavior clearly never existed in the product. If there is _any_ possibility the behavior is a regression (it used to work, or the reporter references a specific version where it worked), use `insufficient` instead and ask for version or timeline information. When in doubt, ask — do not prematurely reclassify. | |||
|
|
|||
There was a problem hiding this comment.
[low] documentation-completeness
The agent prompt instructs to use the effort-estimation skill but provides no explicit fallback guidance if the skill fails. The post-script already handles this via defense-in-depth (defaulting to triaged when effort_requires_review is missing), so no runtime risk exists.
fullsend-ai-review
Bot
added
triage-agent
and removed
requires-manual-review
2 participants
Summary
effort-estimationskill that scores implementation effort (0.25-3 scale) and returns aneffort_requires_reviewboolean controlling whether issues auto-promote to the coder or wait for human revieweffort_requires_reviewinstead of comparing against a hardcoded thresholdCloses fullsend-ai/fullsend#2207
Test plan
bash scripts/post-triage-test.sh-- all 68 cases passeffort-estimationin skills arrayeffort_requires_reviewboolean, not numeric thresholdeffort,effort_requires_review)