fix: weight:0 un-gates criteria and renders as informational#34
Conversation
|
Claude finished @uipreliga's task in 1m 59s —— View job Code Review In Progress
|
akshaylive
left a comment
There was a problem hiding this comment.
The weight 0 fix LGTM. The configurable retry policy should ideally be a separate PR. Additionally the retry policy configuration shouldn't be in "run_limits" (retry policy is not really a limit, so a separate top level field is better). I haven't looked too deep into the impl but would this clear the working directory between retries?
|
The evalboard change moves
|
ee9aa58 to
c746087
Compare
|
@akshaylive thanks — all three points taken. 1. Separate PR. Done. This PR is now the 2. 3. "Would this clear the working directory between retries?" No, it does not — good catch, and it's a real pre-existing gap rather than something the retry knob introduced:
I chose to document this rather than add a reset seam: clearing the sandbox mid-task would discard work the agent had already done and budget already spent, and preserving partials is deliberate (crash forensics). #44 adds a |
- weight: 0 now makes a criterion purely informational — excluded from both the weighted score AND the pass/fail gate (previously it could still flip a task to FAILURE). New BaseSuccessCriterion.is_gating drives the single-source gate (all_criteria_passed) and the `coder-eval evaluate` exit code. A validator rejects weight:0 combined with stop_when or suite_thresholds (arming a non-gating criterion for a pass/fail gate is incoherent). - reports.py failure-reason sampling uses pass_threshold, not score < 1.0. - Doc drift: dropped the non-existent `rephrase` mutation; corrected the stale "max_memory_mb NOT enforced" claim (docker --memory/--cpus/--pids-limit). Split out per review: the configurable retry policy moved to #44, and the evalboard OSS-edition change is dropped entirely (see @bai-uipath's review comment — the panels need core-produced fields that do not exist yet). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
c746087 to
8c02dc6
Compare
Split out of #34 per review, with the placement feedback applied. - New RetryPolicy (models/retry.py): max_retries / initial_delay / backoff_multiplier, all optional. It lives at the task's TOP LEVEL (`retry:`), not under `run_limits` — `run_limits` holds caps that abort a run, while a retry policy is how hard the run tries to survive a transient error. - `retry` is a first-class 4th `-D` root alongside agent/run_limits/sandbox: `-D retry.max_retries=0` to fail fast while debugging. It merges through the SAME resolve_root engine at every layer (experiment defaults, task, variant, CLI), so the unification invariant holds — covered by new test_merge_unification cases. - All retry decisions resolve through one seam, errors/retry.py:: resolve_retry_config. A category that is non-retryable in the built-in table (auth/billing) is never made retryable by a per-run override — retryability is a property of the error, not the run. - Persisted error_details.is_retryable / retry_delay_seconds now reflect the run's actual policy instead of the built-in defaults. - Docs: TASK_DEFINITION_GUIDE gains a `retry` section that documents the placement rationale AND the in-place retry semantics raised in review — a retried communicate() re-sends into the same sandbox and session (the working directory is NOT reset), and the crashed attempt's partial turn is preserved, so trajectory-observing criteria see both attempts. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
@bai-uipath you're right, and I've dropped the evalboard change from this PR entirely rather than argue it. The justification in my PR description — "they derive purely from run artifacts" — was true and answered the wrong question. Portability was the bar I checked; product fit is the bar that matters here, and I didn't check it. Your two specifics hold up when I trace them:
So a new adopter lands on a Watchlist with a dead column and an empty failure-tag panel, and reasonably concludes the page is broken. That's worse than not shipping it, exactly as you said. Dropping it also fixes a packaging problem @akshaylive flagged on the retry policy — this PR is now the If we do want these OSS-side later, the prerequisite is core actually emitting the fields the panels read ( |
akshaylive
left a comment
There was a problem hiding this comment.
🚢 Please update the PR description before merging (squash-merge picks up the PR description into the commit message by default)
Closes the follow-up deferred in #34. `final_status` and both exit codes already ignored weight:0 criteria, but the display sites still labeled a below-threshold informational criterion as FAILED — a header-vs-list contradiction where the task reads SUCCESS while a row reads FAIL. The fix is one carried marker rather than four independent re-derivations: - `CriterionResult.gating` (default True) mirrors `BaseSuccessCriterion. is_gating`, stamped by `SuccessChecker` alongside `pass_threshold` on all three construction paths. Defaulting True means task.json files written before this field read back as gating. - checker log line says "below threshold (informational)" / "errored (informational)" instead of FAILED. - reports_html: rows tagged "informational — not gated (weight: 0)"; the "(n/m passed)" header counts gating criteria only and appends "+ k informational". - reports: suite failure-reason sampling skips non-gating criteria — an informational miss must not explain why a row failed. - evalboard: DTO carries `gating` + `passThreshold`; the pill renders INFO (gray) for non-gating criteria, and the criteria header counts them separately. Drive-by: the pill compared `score === 1`, so a fractional criterion passing at 0.95 with threshold 0.9 rendered FAIL — it now compares against the criterion's own threshold. Tests: gating is stamped from the criterion; results default to gating on read-back; the HTML header excludes informational criteria and labels the row; suite sampling omits an informational miss (verified failing without the fix). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Split out of #34 per review, with the placement feedback applied. - New RetryPolicy (models/retry.py): max_retries / initial_delay / backoff_multiplier, all optional. It lives at the task's TOP LEVEL (`retry:`), not under `run_limits` — `run_limits` holds caps that abort a run, while a retry policy is how hard the run tries to survive a transient error. - `retry` is a first-class 4th `-D` root alongside agent/run_limits/sandbox: `-D retry.max_retries=0` to fail fast while debugging. It merges through the SAME resolve_root engine at every layer (experiment defaults, task, variant, CLI), so the unification invariant holds — covered by new test_merge_unification cases. - All retry decisions resolve through one seam, errors/retry.py:: resolve_retry_config. A category that is non-retryable in the built-in table (auth/billing) is never made retryable by a per-run override — retryability is a property of the error, not the run. - Persisted error_details.is_retryable / retry_delay_seconds now reflect the run's actual policy instead of the built-in defaults. - Docs: TASK_DEFINITION_GUIDE gains a `retry` section that documents the placement rationale AND the in-place retry semantics raised in review — a retried communicate() re-sends into the same sandbox and session (the working directory is NOT reset), and the crashed attempt's partial turn is preserved, so trajectory-observing criteria see both attempts. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Heads-up: a second commit landed on this branch after your reviews, so the approvals here predate it. Why it belongs in this PR: the un-gating fix was only half done. The part that is NOT about const passed = c.score === 1;which ignores I fixed it in place because leaving a threshold-ignoring pill three lines from a threshold-aware one seemed worse than the scope creep — but if you'd rather keep this PR strictly to what you approved, say so and I'll pull those four lines into their own PR.
|

Summary
Makes
weight: 0mean what the docs say it means, end to end.1.
weight: 0un-gates a criterionPreviously a
weight: 0criterion contributed nothing to the weighted score but could still single-handedly flip a task toFAILURE— the docs told authors to also setpass_threshold: 0, which was easy to miss.BaseSuccessCriterion.is_gating(weight > 0.0) is the single source of truth for "does a low score here fail the task".EvaluationResult.all_criteria_passedand thecoder-eval evaluateexit code now gate overis_gatingcriteria only. Aweight: 0miss no longer flipsfinal_statusor the exit code.weight: 0combined withstop_whenorsuite_thresholds— arming a non-gating criterion for a pass/fail gate (early-stop gate, or the across-row suite gate that drives the run exit code) is incoherent and is now caught at load time.2. Display parity (second commit)
final_statusand both exit codes were correct after (1), but every display surface still labeled a below-threshold informational criterion as failed — a header-vs-list contradiction where the task reads SUCCESS while one of its rows reads FAIL. Fixed with one carried marker rather than four re-derivations:CriterionResult.gating(defaultTrue) mirrorsis_gating, stamped bySuccessCheckeralongsidepass_thresholdon all three construction paths. Defaulting True means task.json files written before this field read back as gating.(n/m passed)header counts gating criteria only and appends "+ k informational".gating+passThreshold; non-gating criteria render a grayINFOpill.Note
One change in that commit is not about
weight: 0. The evalboard criterion pill computedpassed = c.score === 1, ignoringpass_thresholdentirely — so any fractional criterion scoring e.g. 0.95 against the default 0.9 threshold rendered FAIL. It now compares against the criterion's own threshold. This affects how every criterion renders in every run, including tasks with noweight: 0anywhere. Called out explicitly because it is outside the scope reviewers approved.3. Doc drift
rephraseprompt mutation from the catalog.max_memory_mbNOT enforced" claim (it maps todocker run --memory;max_cpus/max_pids→--cpus/--pids-limit).reports.pyfailure-reason sampling usespass_threshold, notscore < 1.0.Test plan
make verifygreen — 3442 passed, 2 skipped, 90.91% coverage, ruff + pyright + custom CE lint clean.tsc --noEmitclean on evalboard.weight: 0un-gating semantics, thestop_when/suite_thresholdsrejections, acoder-eval evaluateexit-code test (with fixture) proving an informational miss doesn't fail the run, and for the display work —gatingstamped from the criterion, default-True read-back, the HTML header/row labeling, and suite sampling omitting an informational miss (verified failing without the fix).Split out of this PR, per review
retry:root #44, per @akshaylive. Also moved out ofrun_limitsto a top-levelretry:root there, per the same review./trendsand/watchlistclear portability but not product fit —mature_skippedhas no producer insrc/, anddominantFailureTagsneeds the review index, which the OSS core doesn't write.🤖 Generated with Claude Code