fix: close path-traversal gap and harness scoring nits from full code review - #169
Closed
akshaylive wants to merge 1 commit into
Closed
akshaylive wants to merge 1 commit into
akshaylive wants to merge 1 commit into
Conversation
… review Ran the full 8-axis coder-eval-code-review-full pass and fixed the mechanically-safe, high-confidence findings: - orchestration/batch.py::clear_rerun_artifacts joined an unvalidated task_id onto the run dir and rmtree'd it with no containment check, letting a crafted task_id delete outside the run directory. Extracted regrade.py's containment check into a shared path_utils.is_within and applied it here too, with a regression test. - criteria/llm_judge.py's route-dispatch match had no exhaustiveness arm (confirmed py/uninitialized-local-variable CodeQL alert); added the same case _: raise AssertionError(...) pattern already used in models/routing.py::resolve_route. - reports.py's per-criterion completion_rate published a bare 0.0 for a zero-row suite instead of following the nothing_was_measured convention pass_rate/error_share already use; now omitted so a suite_thresholds gate fails closed with a visible actual_value=None. - batch.py's cost-pricing-coverage check used an untyped getattr(criterion, "model", None) probe; replaced with isinstance narrowing against LLMJudgeCriterion, the only union member with that field. - Removed a stale optimize/ line from CLAUDE.md's directory tree. Larger findings (docker image-tag collisions under parallel builds, CE039 discipline gaps across 5 criteria checkers, generate_run_id collision risk, UiPath credentials forwarded by default, a 3443-line orchestrator.py) are documented in tmp/code-review-20260914-124404/ for dedicated follow-up rather than bundled here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
akshaylive
requested review from
CarlesUIPath,
bai-uipath,
tmatup and
uipreliga
as code owners
September 14, 2026 19:47
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
Security — path traversal in
--resumecleanup.orchestration/batch.py::clear_rerun_artifactsjoined the task-authoredtask_id(an unvalidated string; dataset rows use"<suite>/<row>") straight onto the run directory andrmtree'd the result with no containment check. A craftedtask_idlike"../../victim"could delete a directory outside the run tree. Pulled the containment checkorchestration/regrade.pyalready applies to the same shape of field into a sharedpath_utils.is_within, and used it at both call sites. Addedtest_clear_rerun_artifacts_refuses_path_traversalto lock it in.Correctness — unbound-variable risk in the judge's route dispatch.
criteria/llm_judge.py::_invoke_tool_channelmatches onrouteand unconditionally readsverdict/err/response_usageafterward, but thematchhad no wildcard arm — flagged by CodeQL (py/uninitialized-local-variable) and already fixed once before in the siblingmodels/routing.py::resolve_route. Added the samecase _: raise AssertionError(...)arm here.Correctness — misleading zero in report scoring.
reports.py's per-criterioncompletion_ratecomputedrows_aggregated / rows_total if rows_total else 0.0, so a zero-row suite published0.0— indistinguishable from "measured and completely failed."pass_rate/error_sharealready avoid this vianothing_was_measured;completion_ratenow follows suit by omitting the metric when there's no denominator, so asuite_thresholdsgate on it fails closed withactual_value=Noneinstead of a silent zero.Type safety — rename-unsafe cost probe.
batch.py's pricing-coverage check usedgetattr(criterion, "model", None), which would silently start returningNoneforever ifLLMJudgeCriterion.modelwere ever renamed. Replaced withisinstance(criterion, LLMJudgeCriterion)narrowing.Docs. Removed a stale
optimize/line from CLAUDE.md's directory tree — the package doesn't exist anymore.Context
Found via a full 8-axis code review (
/coder-eval-code-review-full, all axes, no scope filter — report kept locally under gitignoredtmp/, not part of this diff). These five are the findings that were both real and safely fixable without a design call. Deliberately left out, each needing its own decision: docker image tags colliding under parallel variant builds, the same checker-misuse-vs-agent-failure gap (CE039) in 5 more criteria,generate_run_id()'s same-second collision risk, UiPath credentials forwarded into every sandbox by default, andorchestrator.pybeing a 3443-line god class. One reviewed candidate — LiteLLM route allegedly missingtemperature— turned out to be intentional per its own docstring; no change made.Testing
Full suite: 6167 passed, 8 skipped, 0 failed.
ruff/pyright/all 635 CE0xx lint checks clean.🤖 Generated with Claude Code